From 0847d7d6188914c96794c9a7aab78a657b62f355 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. This issue is fixed by making use of CFileAttrLock which makes it possible to update file properties without requiring the file to be locked. Changes: - add addReadCost and addNumDiskRead to IDistributedFile interface - Implementation of addReadCost and addNumDiskRead makes use of CFileAttrLock to lock the file properties and make the necessary updates - Make FileSprayer::updateTargetProperties update only the target properties (it was also previously updating the source properties) - Create FileSprayer::updateSourceProperties to update the source properties and make it use IDistributedFile::addReadCost and IDistributedFile::addNumDiskRead to make the necessary updates, removing the need to lock the IDistributedFile. Signed-off-by: Shamser Ahmed --- dali/base/dadfs.cpp | 27 +++++++++++++++++++ dali/base/dadfs.hpp | 2 ++ dali/ft/filecopy.cpp | 32 ++++++++++++----------- dali/ft/filecopy.ipp | 3 ++- esp/clients/ws_dfsclient/ws_dfsclient.cpp | 8 ++++++ 5 files changed, 56 insertions(+), 16 deletions(-) diff --git a/dali/base/dadfs.cpp b/dali/base/dadfs.cpp index e22e5c7b487..517d30e92ee 100644 --- a/dali/base/dadfs.cpp +++ b/dali/base/dadfs.cpp @@ -3282,6 +3282,33 @@ class CDistributedFileBase : implements INTERFACE, public CInterface setAccessedTime(dt); } + virtual void addNumDiskRead(stat_type numReads) override + { + if (!logicalName.isForeign() && numReads) + { + CFileAttrLock attrLock; + if (conn) + lockFileAttrLock(attrLock); + + stat_type prevNumReads = queryAttributes().getPropInt64(getDFUQResultFieldName(DFUQRFnumDiskReads), 0); + queryAttributes().setPropInt64(getDFUQResultFieldName(DFUQRFnumDiskReads), prevNumReads + numReads); + } + } + + virtual void addReadCost(stat_type readCost) override + { + if (!logicalName.isForeign() && readCost) + { + CFileAttrLock attrLock; + if (conn) + lockFileAttrLock(attrLock); + IPropertyTree &attrs = queryAttributes(); + cost_type legacyReadCost = getLegacyReadCost(attrs, this); + cost_type prevReadCost = attrs.getPropInt64(getDFUQResultFieldName(DFUQRFreadCost), 0); + attrs.setPropInt64(getDFUQResultFieldName(DFUQRFreadCost), legacyReadCost + prevReadCost + readCost); + } + } + virtual void addAttrValue(const char *attr, unsigned __int64 value) override { if (0==value) diff --git a/dali/base/dadfs.hpp b/dali/base/dadfs.hpp index 6cde9f34875..be51efcfc32 100644 --- a/dali/base/dadfs.hpp +++ b/dali/base/dadfs.hpp @@ -366,6 +366,8 @@ interface IDistributedFile: extends IInterface virtual bool getAccessedTime(CDateTime &dt) = 0; // get date and time last accessed (returns false if not set) virtual void setAccessedTime(const CDateTime &dt) = 0; // set date and time last accessed virtual void setAccessed() = 0; // set date and time last accessed to now (local time) + virtual void addNumDiskRead(stat_type numReads) = 0; // add to number of disk reads + virtual void addReadCost(stat_type readCost) = 0; // add to read costs virtual void addAttrValue(const char *attr, unsigned __int64 value) = 0; // atomic add to attribute value virtual unsigned numCopies(unsigned partno) = 0; // number of copies diff --git a/dali/ft/filecopy.cpp b/dali/ft/filecopy.cpp index ad88915c46c..06ab6211442 100644 --- a/dali/ft/filecopy.cpp +++ b/dali/ft/filecopy.cpp @@ -1512,6 +1512,7 @@ void FileSprayer::checkForOverlap() void FileSprayer::cleanupRecovery() { progressTree->setPropBool(ANcomplete, true); + #ifdef CLEANUP_RECOVERY progressTree->removeProp(ANhasPartition); progressTree->removeProp(ANhasProgress); @@ -3396,7 +3397,10 @@ 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 = 0, totalReadCost = 0; + updateTargetProperties(totalWriteCost); + updateSourceProperties(totalReadCost); + progressReport->setFileAccessCost(totalReadCost+totalWriteCost); StringBuffer copyEventText; // [logical-source] > [logical-target] if (distributedSource) @@ -3446,7 +3450,7 @@ bool FileSprayer::isSameSizeHeaderFooter() return retVal; } -void FileSprayer::updateTargetProperties() +void FileSprayer::updateTargetProperties(cost_type & totalReadCost) { TimeSection timer("FileSprayer::updateTargetProperties() time"); Owned error; @@ -3804,9 +3808,16 @@ void FileSprayer::updateTargetProperties() if (expireDays != -1) curProps.setPropInt("@expireDays", expireDays); } + if (error) + throw error.getClear(); +} + + + +void FileSprayer::updateSourceProperties(cost_type & totalReadCost) +{ + 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) { IDistributedSuperFile * superSrc = distributedSource->querySuperFile(); @@ -3854,20 +3865,11 @@ 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->addNumDiskRead(totalNumReads); + distributedSource->addReadCost(totalReadCost); } - progressReport->setFileAccessCost(totalReadCost+totalWriteCost); - if (error) - throw error.getClear(); } - void FileSprayer::splitAndCollectFileInfo(IPropertyTree * newRecord, RemoteFilename &remoteFileName, bool isDistributedSource) { diff --git a/dali/ft/filecopy.ipp b/dali/ft/filecopy.ipp index e6005431304..ce1b1da7c48 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(); + void updateTargetProperties(cost_type & readCost); + void updateSourceProperties(cost_type & WriteCost); bool usePullOperation() const; bool usePushOperation() const; bool usePushWholeOperation() const; diff --git a/esp/clients/ws_dfsclient/ws_dfsclient.cpp b/esp/clients/ws_dfsclient/ws_dfsclient.cpp index 81a845665a1..315a958daa2 100644 --- a/esp/clients/ws_dfsclient/ws_dfsclient.cpp +++ b/esp/clients/ws_dfsclient/ws_dfsclient.cpp @@ -263,6 +263,14 @@ class CServiceDistributedFileBase : public CSimpleInterfaceOf { legacyDFSFile->setAccessed(); } + virtual void addNumDiskRead(stat_type numReads) override + { + legacyDFSFile->addNumDiskRead(numReads); + } + virtual void addReadCost(stat_type numReads) override + { + legacyDFSFile->addReadCost(numReads); + } virtual void addAttrValue(const char *attr, unsigned __int64 value) override { legacyDFSFile->addAttrValue(attr, value);