From 5f8ae0d399bd27456fdc72163683ca19218299ee Mon Sep 17 00:00:00 2001 From: Brian Bockelman Date: Mon, 19 Nov 2018 10:58:59 -0600 Subject: [PATCH] [POSIX] Avoid SEGV due to race condition between open() and close(). This should protect against a SEGV observed under heavy load for file caches. This is a backport of ae1fcd1142a7570dd97473edb54555d63b5377f0 Author: abh3 --- src/XrdPosix/XrdPosixCallBack.hh | 2 +- src/XrdPosix/XrdPosixFile.cc | 85 +++++++++++++++++++++++++------- src/XrdPosix/XrdPosixFile.hh | 11 ++--- src/XrdPosix/XrdPosixFileRH.cc | 1 + src/XrdPosix/XrdPosixPrepIO.cc | 22 +++++++-- src/XrdPosix/XrdPosixPrepIO.hh | 2 + src/XrdPosix/XrdPosixXrootd.cc | 2 +- 7 files changed, 94 insertions(+), 31 deletions(-) diff --git a/src/XrdPosix/XrdPosixCallBack.hh b/src/XrdPosix/XrdPosixCallBack.hh index 01794387de6..6c46571aa56 100644 --- a/src/XrdPosix/XrdPosixCallBack.hh +++ b/src/XrdPosix/XrdPosixCallBack.hh @@ -59,7 +59,7 @@ virtual ~XrdPosixCallBack() {} }; //----------------------------------------------------------------------------- -//! @brief An abstract class to define a callback for open file requests. +//! @brief An abstract class to define a callback for file I/O requests. //! //! This abstract class defines the callback interface for Fsync(), Pread(), //! Pwrite(), and VRead(). Async I/O is not supported for Read(), Readv(), diff --git a/src/XrdPosix/XrdPosixFile.cc b/src/XrdPosix/XrdPosixFile.cc index 3d04bbfd4ad..c41ed58f459 100644 --- a/src/XrdPosix/XrdPosixFile.cc +++ b/src/XrdPosix/XrdPosixFile.cc @@ -243,6 +243,11 @@ void XrdPosixFile::DelayedDestroy(XrdPosixFile *fp) bool XrdPosixFile::Close(XrdCl::XRootDStatus &Status) { +// If this is a defered open, disable any future calls as we are ready to +// shutdown this beast! +// + if (PrepIO) PrepIO->Disable(); + // If we don't need to close the file, then return success. Otherwise, do the // actual close and return the status. We should have already been removed // from the file table at this point and should be unlocked. @@ -330,7 +335,7 @@ void XrdPosixFile::HandleResponse(XrdCl::XRootDStatus *status, // delete status; delete response; - if (rc) delete this; + if (rc < 0) delete this; } /******************************************************************************/ @@ -366,9 +371,11 @@ int XrdPosixFile::Read (char *Buff, long long Offs, int Len) XrdCl::XRootDStatus Status; uint32_t bytes; -// Issue read and return appropriately +// Issue read and return appropriately. // + Ref(); Status = clFile.Read((uint64_t)Offs, (uint32_t)Len, Buff, bytes); + unRef(); return (Status.IsOK() ? (int)bytes : XrdPosixMap::Result(Status)); } @@ -384,11 +391,15 @@ void XrdPosixFile::Read (XrdOucCacheIOCB &iocb, char *buff, long long offs, // Issue read // + Ref(); Status = clFile.Read((uint64_t)offs, (uint32_t)rlen, buff, rhp); // Check status // - if (!Status.IsOK()) rhp->Sched(-XrdPosixMap::Result(Status)); + if (!Status.IsOK()) + {rhp->Sched(-XrdPosixMap::Result(Status)); + unRef(); + } } /******************************************************************************/ @@ -416,7 +427,9 @@ int XrdPosixFile::ReadV (const XrdOucIOVec *readV, int n) // Issue the readv. We immediately delete the vrInfo as w don't need it as a // readv will succeed only if actually read the number of bytes requested. // + Ref(); Status = clFile.VectorRead(chunkVec, (void *)0, vrInfo); + unRef(); delete vrInfo; // Return appropriate result @@ -447,11 +460,15 @@ void XrdPosixFile::ReadV(XrdOucCacheIOCB &iocb, const XrdOucIOVec *readV, int n) // XrdPosixFileRH *rhp = XrdPosixFileRH::Alloc(&iocb, this, 0, nbytes, XrdPosixFileRH::isReadV); + Ref(); Status = clFile.VectorRead(chunkVec, (void *)0, rhp); // Return appropriate result // - if (!Status.IsOK()) rhp->Sched(-XrdPosixMap::Result(Status)); + if (!Status.IsOK()) + {rhp->Sched(-XrdPosixMap::Result(Status)); + unRef(); + } } /******************************************************************************/ @@ -464,9 +481,11 @@ bool XrdPosixFile::Stat(XrdCl::XRootDStatus &Status, bool force) // Get the stat information from the open file // + Ref(); Status = clFile.Stat(force, sInfo); if (!Status.IsOK()) - {delete sInfo; + {unRef(); + delete sInfo; return false; } @@ -479,6 +498,7 @@ bool XrdPosixFile::Stat(XrdCl::XRootDStatus &Status, bool force) // Delete our status information and return final result // + unRef(); delete sInfo; return true; } @@ -487,6 +507,23 @@ bool XrdPosixFile::Stat(XrdCl::XRootDStatus &Status, bool force) /* S y n c */ /******************************************************************************/ +int XrdPosixFile::Sync() +{ + XrdCl::XRootDStatus Status; + +// Issue the Sync +// + Ref(); + Status = clFile.Sync(); + unRef(); + +// Return result +// + return XrdPosixMap::Result(Status); +} + +/******************************************************************************/ + void XrdPosixFile::Sync(XrdOucCacheIOCB &iocb) { XrdCl::XRootDStatus Status; @@ -502,6 +539,25 @@ void XrdPosixFile::Sync(XrdOucCacheIOCB &iocb) if (!Status.IsOK()) rhp->Sched(-XrdPosixMap::Result(Status)); } +/******************************************************************************/ +/* T r u n c */ +/******************************************************************************/ + +int XrdPosixFile::Trunc(long long Offset) +{ + XrdCl::XRootDStatus Status; + +// Issue truncate request +// + Ref(); + Status = clFile.Truncate((uint64_t)Offset); + unRef(); + +// Return results +// + return XrdPosixMap::Result(Status); +} + /******************************************************************************/ /* W r i t e */ /******************************************************************************/ @@ -512,7 +568,9 @@ int XrdPosixFile::Write(char *Buff, long long Offs, int Len) // Issue read and return appropriately // + Ref(); Status = clFile.Write((uint64_t)Offs, (uint32_t)Len, Buff); + unRef(); return (Status.IsOK() ? Len : XrdPosixMap::Result(Status)); } @@ -528,20 +586,13 @@ void XrdPosixFile::Write(XrdOucCacheIOCB &iocb, char *buff, long long offs, // Issue read // + Ref(); Status = clFile.Write((uint64_t)offs, (uint32_t)wlen, buff, rhp); // Check status // - if (!Status.IsOK()) rhp->Sched(-XrdPosixMap::Result(Status)); -} - -/******************************************************************************/ -/* D o I t */ -/******************************************************************************/ -void XrdPosixFile::DoIt() -{ -// Virtual function of XrdJob. -// Called from XrdPosixXrootd::Close if the file is still IO active. - - delete this; + if (!Status.IsOK()) + {rhp->Sched(-XrdPosixMap::Result(Status)); + unRef(); + } } diff --git a/src/XrdPosix/XrdPosixFile.hh b/src/XrdPosix/XrdPosixFile.hh index 3ab6ca9dbf7..d8fa6739680 100644 --- a/src/XrdPosix/XrdPosixFile.hh +++ b/src/XrdPosix/XrdPosixFile.hh @@ -47,7 +47,6 @@ #include "XrdPosix/XrdPosixMap.hh" #include "XrdPosix/XrdPosixObject.hh" -#include "Xrd/XrdJob.hh" /******************************************************************************/ /* X r d P o s i x F i l e C l a s s */ /******************************************************************************/ @@ -57,8 +56,7 @@ class XrdPosixPrepIO; class XrdPosixFile : public XrdPosixObject, public XrdOucCacheIO2, - public XrdCl::ResponseHandler, - public XrdJob + public XrdCl::ResponseHandler { public: @@ -130,12 +128,11 @@ static void DelayedDestroy(XrdPosixFile *fp); bool Stat(XrdCl::XRootDStatus &Status, bool force=false); - int Sync() {return XrdPosixMap::Result(clFile.Sync());} + int Sync(); void Sync(XrdOucCacheIOCB &iocb); - int Trunc(long long Offset) - {return XrdPosixMap::Result(clFile.Truncate((uint64_t)Offset));} + int Trunc(long long Offset); void UpdtSize(size_t newsz) {updMutex.Lock(); @@ -153,8 +150,6 @@ static void DelayedDestroy(XrdPosixFile *fp); void Write(XrdOucCacheIOCB &iocb, char *buff, long long offs, int wlen); - void DoIt(); - size_t mySize; time_t myMtime; dev_t myRdev; diff --git a/src/XrdPosix/XrdPosixFileRH.cc b/src/XrdPosix/XrdPosixFileRH.cc index 0d05154a634..a1dc2a09875 100644 --- a/src/XrdPosix/XrdPosixFileRH.cc +++ b/src/XrdPosix/XrdPosixFileRH.cc @@ -121,6 +121,7 @@ void XrdPosixFileRH::HandleResponse(XrdCl::XRootDStatus *status, // Now schedule this callback // + theFile->unRef(); if (XrdPosixGlobals::schedP) XrdPosixGlobals::schedP->Schedule(this); else {pthread_t tid; XrdSysThread::Run(&tid, callDoIt, this, 0, "PosixFileRH"); diff --git a/src/XrdPosix/XrdPosixPrepIO.cc b/src/XrdPosix/XrdPosixPrepIO.cc index 2d88ead52a0..8df1c936b10 100644 --- a/src/XrdPosix/XrdPosixPrepIO.cc +++ b/src/XrdPosix/XrdPosixPrepIO.cc @@ -32,6 +32,20 @@ #include "XrdPosix/XrdPosixPrepIO.hh" #include "XrdPosix/XrdPosixTrace.hh" +/******************************************************************************/ +/* D i s a b l e */ +/******************************************************************************/ + +void XrdPosixPrepIO::Disable() +{ + EPNAME("PrepIODisable"); + XrdPosixObjGuard objGuard(fileP); + + DEBUG("Disabling defered open "<Origin()); + + openRC = -ESHUTDOWN; +} + /******************************************************************************/ /* I n i t */ /******************************************************************************/ @@ -51,14 +65,14 @@ bool XrdPosixPrepIO::Init(XrdOucCacheIOCB *iocbP) DMSG("Init", iCalls <<" unexpected PrepIO calls!"); } -// Check if the file is already opened. This caller may be vestigial -// - if (fileP->clFile.IsOpen()) return true; - // Do not try to open the file if there was previous error // if (openRC) return false; +// Check if the file is already opened. This caller may be vestigial +// + if (fileP->clFile.IsOpen()) return true; + // Open the file. It is too difficult to do an async open here as there is a // possible pending async request and doing both is not easy at all. // diff --git a/src/XrdPosix/XrdPosixPrepIO.hh b/src/XrdPosix/XrdPosixPrepIO.hh index 42d601b91ea..e3fda717f73 100644 --- a/src/XrdPosix/XrdPosixPrepIO.hh +++ b/src/XrdPosix/XrdPosixPrepIO.hh @@ -41,6 +41,8 @@ XrdOucCacheIO *Base() {return this;} // Already defined XrdOucCacheIO *Detach() {return this;} // Already defined +void Disable(); + long long FSize() {return (Init() ? fileP->FSize() : openRC);} int Fstat(struct stat &buf) diff --git a/src/XrdPosix/XrdPosixXrootd.cc b/src/XrdPosix/XrdPosixXrootd.cc index 7c49cdf775f..bc76fe325d4 100644 --- a/src/XrdPosix/XrdPosixXrootd.cc +++ b/src/XrdPosix/XrdPosixXrootd.cc @@ -264,7 +264,7 @@ int XrdPosixXrootd::Close(int fildes) // Close the file if there is no active I/O (possible caching). Delete the // object if the close was successful (it might not be). // - if (!fP->Refs() && !(fP->XCio->ioActive())) + if (!(fP->XCio->ioActive()) && !fP->Refs()) {if ((ret = fP->Close(Status))) {delete fP; fP = 0;} else {DEBUG(Status.ToString().c_str() <<" closing " <Origin());} } else ret = true;