From a60035cb1acdf7ea584f99b566338176dc25ca31 Mon Sep 17 00:00:00 2001 From: Shamser Ahmed Date: Thu, 24 Oct 2024 10:20:25 +0100 Subject: [PATCH] HPCC-32803 In file ops, update source file props without locking source file This fixes the issue of some file operations failing because the source file was locked when the operation was taking place. Source files may be locked if it is being processed elsewhere. Prevously, the source file was being locked to allow the source file's disk read count and read costs properties to be updated. The fix makes use of CFileAttrLock to update file properties without requiring the file to be locked. Changes: - Make FileSprayer::updateTargetProperties update only update the target properties (it was also previously updating the source properties) - Create FileSprayer::updateSourceProperties to update the source properties and make it use CFileAttrLock lock the attributes, removing the need to lock the IDistributedFile. Signed-off-by: Shamser Ahmed --- dali/ft/filecopy.cpp | 46 ++++++++++++++++++++++---------------------- dali/ft/filecopy.ipp | 3 ++- 2 files changed, 25 insertions(+), 24 deletions(-) diff --git a/dali/ft/filecopy.cpp b/dali/ft/filecopy.cpp index ad88915c46c..7f11032418e 100644 --- a/dali/ft/filecopy.cpp +++ b/dali/ft/filecopy.cpp @@ -3396,7 +3396,9 @@ void FileSprayer::spray() //If got here then we have succeeded //Note: On failure, costs will not be updated. Future: would be useful to have a way to update costs on failure. - updateTargetProperties(); + cost_type totalWriteCost = updateTargetProperties(); + cost_type totalReadCost = updateSourceProperties(); + progressReport->setFileAccessCost(totalReadCost+totalWriteCost); StringBuffer copyEventText; // [logical-source] > [logical-target] if (distributedSource) @@ -3446,13 +3448,13 @@ bool FileSprayer::isSameSizeHeaderFooter() return retVal; } -void FileSprayer::updateTargetProperties() +cost_type FileSprayer::updateTargetProperties() { TimeSection timer("FileSprayer::updateTargetProperties() time"); Owned error; - cost_type totalWriteCost = 0; if (distributedTarget) { + cost_type totalWriteCost = 0; StringBuffer failedParts; CRC32Merger partCRC; offset_t partLength = 0; @@ -3803,12 +3805,20 @@ void FileSprayer::updateTargetProperties() int expireDays = options->getPropInt("@expireDays", -1); if (expireDays != -1) curProps.setPropInt("@expireDays", expireDays); + return totalWriteCost; } + if (error) + throw error.getClear(); + return 0; +} + +cost_type FileSprayer::updateSourceProperties() +{ + TimeSection timer("FileSprayer::updateSourceProperties() time"); // Update file readCost and numReads in file properties and do the same for subfiles - // Update totalReadCost - cost_type totalReadCost = 0; if (distributedSource) { + cost_type totalReadCost = 0; IDistributedSuperFile * superSrc = distributedSource->querySuperFile(); if (superSrc && superSrc->numSubFiles() > 0) { @@ -3833,14 +3843,10 @@ void FileSprayer::updateTargetProperties() // so query the first (and only) subfile subfile = &superSrc->querySubFile(0); } - DistributedFilePropertyLock lock(subfile); - IPropertyTree &subFileProps = lock.queryAttributes(); - stat_type prevNumReads = subFileProps.getPropInt64(getDFUQResultFieldName(DFUQRFnumDiskReads), 0); - cost_type legacyReadCost = getLegacyReadCost(subfile->queryAttributes(), subfile); - cost_type prevReadCost = subFileProps.getPropInt64(getDFUQResultFieldName(DFUQRFreadCost), 0); cost_type curReadCost = calcFileAccessCost(subfile, 0, curProgress.numReads); - subFileProps.setPropInt64(getDFUQResultFieldName(DFUQRFnumDiskReads), prevNumReads + curProgress.numReads); - subFileProps.setPropInt64(getDFUQResultFieldName(DFUQRFreadCost), legacyReadCost + prevReadCost + curReadCost); + subfile->addAttrValue(getDFUQResultFieldName(DFUQRFnumDiskReads), curProgress.numReads); + cost_type legacyReadCost = getLegacyReadCost(subfile->queryAttributes(), subfile); + subfile->addAttrValue(getDFUQResultFieldName(DFUQRFreadCost), curReadCost); totalReadCost += curReadCost; } else @@ -3854,20 +3860,14 @@ void FileSprayer::updateTargetProperties() { totalReadCost = calcFileAccessCost(distributedSource, 0, totalNumReads); } - DistributedFilePropertyLock lock(distributedSource); - IPropertyTree &curProps = lock.queryAttributes(); - stat_type prevNumReads = curProps.getPropInt64(getDFUQResultFieldName(DFUQRFnumDiskReads), 0); - cost_type legacyReadCost = getLegacyReadCost(curProps, distributedSource); - cost_type prevReadCost = curProps.getPropInt64(getDFUQResultFieldName(DFUQRFreadCost), 0); - curProps.setPropInt64(getDFUQResultFieldName(DFUQRFnumDiskReads), prevNumReads + totalNumReads); - curProps.setPropInt64(getDFUQResultFieldName(DFUQRFreadCost), legacyReadCost + prevReadCost + totalReadCost); + distributedSource->addAttrValue(getDFUQResultFieldName(DFUQRFnumDiskReads), totalNumReads); + cost_type legacyReadCost = getLegacyReadCost(distributedSource->queryAttributes(), distributedSource); + distributedSource->addAttrValue(getDFUQResultFieldName(DFUQRFreadCost), legacyReadCost + totalReadCost); + return totalReadCost; } - progressReport->setFileAccessCost(totalReadCost+totalWriteCost); - if (error) - throw error.getClear(); + return 0; } - void FileSprayer::splitAndCollectFileInfo(IPropertyTree * newRecord, RemoteFilename &remoteFileName, bool isDistributedSource) { diff --git a/dali/ft/filecopy.ipp b/dali/ft/filecopy.ipp index e6005431304..b166469e7c1 100644 --- a/dali/ft/filecopy.ipp +++ b/dali/ft/filecopy.ipp @@ -271,7 +271,8 @@ protected: void savePartition(); void setCopyCompressedRaw(); void setSource(IFileDescriptor * source, unsigned copy, unsigned mirrorCopy = (unsigned)-1); - void updateTargetProperties(); + cost_type updateTargetProperties(); + cost_type updateSourceProperties(); bool usePullOperation() const; bool usePushOperation() const; bool usePushWholeOperation() const;