Skip to content

Commit

Permalink
HPCC-32803 In file ops, update source file props without locking sour…
Browse files Browse the repository at this point in the history
…ce 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 <[email protected]>
  • Loading branch information
shamser committed Oct 25, 2024
1 parent d4c4918 commit d7e4efb
Show file tree
Hide file tree
Showing 2 changed files with 20 additions and 23 deletions.
40 changes: 18 additions & 22 deletions dali/ft/filecopy.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -3396,7 +3396,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)
Expand Down Expand Up @@ -3446,7 +3449,7 @@ bool FileSprayer::isSameSizeHeaderFooter()
return retVal;
}

void FileSprayer::updateTargetProperties()
void FileSprayer::updateTargetProperties(cost_type & totalReadCost)
{
TimeSection timer("FileSprayer::updateTargetProperties() time");
Owned<IException> error;
Expand Down Expand Up @@ -3804,9 +3807,14 @@ 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();
Expand All @@ -3833,14 +3841,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
Expand All @@ -3854,20 +3858,12 @@ 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);
}
progressReport->setFileAccessCost(totalReadCost+totalWriteCost);
if (error)
throw error.getClear();
}


void FileSprayer::splitAndCollectFileInfo(IPropertyTree * newRecord, RemoteFilename &remoteFileName,
bool isDistributedSource)
{
Expand Down
3 changes: 2 additions & 1 deletion dali/ft/filecopy.ipp
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down

0 comments on commit d7e4efb

Please sign in to comment.