From 5a98f2d50913682c4ebe0e0c4ce695c450a96091 Mon Sep 17 00:00:00 2001 From: Joey Hess Date: Mon, 16 May 2022 12:34:56 -0400 Subject: [PATCH] avoid creating content directory when locking content If the content directory does not exist, then it does not make sense to lock the content file, as it also does not exist, and so it's ok for the lock operation to fail. This avoids potential races where the content file exists but is then deleted/renamed, while another process sees that it exists and goes to lock it, resulting in a dangling lock file in an otherwise empty object directory. Also renamed modifyContent to modifyContentDir since it is not only necessarily used for modifying content files, but also other files in the content directory. Sponsored-by: Dartmouth College's Datalad project --- Annex/Content.hs | 10 +++---- Annex/Content/Presence.hs | 2 +- Annex/Perms.hs | 20 ++++++++++---- Command/Fix.hs | 2 +- Command/Lock.hs | 4 +-- Upgrade/V5/Direct.hs | 4 +-- ..._e8ab74bb4951fe7f0c71630bb278ad6f._comment | 26 +++++++++++++++++++ 7 files changed, 52 insertions(+), 16 deletions(-) create mode 100644 doc/todo/command_to___34__migrate__34___from_adjusted_mode/comment_6_e8ab74bb4951fe7f0c71630bb278ad6f._comment diff --git a/Annex/Content.hs b/Annex/Content.hs index 141f8e206a..c81dc5bff6 100644 --- a/Annex/Content.hs +++ b/Annex/Content.hs @@ -169,13 +169,13 @@ type ContentLocker = RawFilePath -> Maybe LockFile -> (Annex (Maybe LockHandle), posixLocker :: (Maybe FileMode -> LockFile -> Annex (Maybe LockHandle)) -> LockFile -> Annex (Maybe LockHandle) posixLocker takelock lockfile = do mode <- annexFileMode - modifyContent lockfile $ + modifyContentDirWhenExists lockfile $ takelock (Just mode) lockfile #else winLocker :: (LockFile -> IO (Maybe LockHandle)) -> ContentLocker winLocker takelock _ (Just lockfile) = let lck = do - modifyContent lockfile $ + modifyContentDirWhenExists lockfile $ void $ liftIO $ tryIO $ writeFile (fromRawFilePath lockfile) "" liftIO $ takelock lockfile @@ -407,7 +407,7 @@ moveAnnex key af src = ifM (checkSecureHashes' key) where storeobject dest = ifM (liftIO $ R.doesPathExist dest) ( alreadyhave - , adjustedBranchRefresh af $ modifyContent dest $ do + , adjustedBranchRefresh af $ modifyContentDir dest $ do liftIO $ moveFile (fromRawFilePath src) (fromRawFilePath dest) @@ -452,7 +452,7 @@ linkToAnnex :: Key -> RawFilePath -> Maybe InodeCache -> Annex LinkAnnexResult linkToAnnex key src srcic = ifM (checkSecureHashes' key) ( do dest <- calcRepo (gitAnnexLocation key) - modifyContent dest $ linkAnnex To key src srcic dest Nothing + modifyContentDir dest $ linkAnnex To key src srcic dest Nothing , return LinkAnnexFailed ) @@ -528,7 +528,7 @@ linkAnnex fromto key src (Just srcic) dest destmode = unlinkAnnex :: Key -> Annex () unlinkAnnex key = do obj <- calcRepo (gitAnnexLocation key) - modifyContent obj $ do + modifyContentDir obj $ do secureErase obj liftIO $ removeWhenExistsWith R.removeLink obj diff --git a/Annex/Content/Presence.hs b/Annex/Content/Presence.hs index 48c8b92873..86fde17e44 100644 --- a/Annex/Content/Presence.hs +++ b/Annex/Content/Presence.hs @@ -112,7 +112,7 @@ inAnnexSafe key = inAnnex' (fromMaybe True) (Just False) go key - remove the lock file to clean up after ourselves. -} checklock (Just lockfile) contentfile = ifM (liftIO $ doesFileExist (fromRawFilePath contentfile)) - ( modifyContent lockfile $ liftIO $ + ( modifyContentDirWhenExists lockfile $ liftIO $ lockShared lockfile >>= \case Nothing -> return is_locked Just lockhandle -> do diff --git a/Annex/Perms.hs b/Annex/Perms.hs index 4cecc48e79..5e3de2d16a 100644 --- a/Annex/Perms.hs +++ b/Annex/Perms.hs @@ -26,7 +26,8 @@ module Annex.Perms ( createContentDir, freezeContentDir, thawContentDir, - modifyContent, + modifyContentDir, + modifyContentDirWhenExists, withShared, hasFreezeHook, hasThawHook, @@ -290,15 +291,24 @@ createContentDir dest = do dir = parentDir dest {- Creates the content directory for a file if it doesn't already exist, - - or thaws it if it does, then runs an action to modify the file, and - - finally, freezes the content directory. -} -modifyContent :: RawFilePath -> Annex a -> Annex a -modifyContent f a = do + - or thaws it if it does, then runs an action to modify a file in the + - directory, and finally, freezes the content directory. -} +modifyContentDir :: RawFilePath -> Annex a -> Annex a +modifyContentDir f a = do createContentDir f -- also thaws it v <- tryNonAsync a freezeContentDir f either throwM return v +{- Like modifyContentDir, but avoids creating the content directory if it + - ddoes not already exist. In that case, the action will probably fail. -} +modifyContentDirWhenExists :: RawFilePath -> Annex a -> Annex a +modifyContentDirWhenExists f a = do + thawContentDir f + v <- tryNonAsync a + freezeContentDir f + either throwM return v + hasFreezeHook :: Annex Bool hasFreezeHook = isJust . annexFreezeContentCommand <$> Annex.getGitConfig diff --git a/Command/Fix.hs b/Command/Fix.hs index 109b17389e..581cb7ced2 100644 --- a/Command/Fix.hs +++ b/Command/Fix.hs @@ -78,7 +78,7 @@ breakHardLink file key obj = do error "unable to break hard link" thawContent tmp' Database.Keys.storeInodeCaches key [tmp'] - modifyContent obj $ freezeContent obj + modifyContentDir obj $ freezeContent obj next $ return True makeHardLink :: RawFilePath -> Key -> CommandPerform diff --git a/Command/Lock.hs b/Command/Lock.hs index 620da40133..e0d4b8f885 100644 --- a/Command/Lock.hs +++ b/Command/Lock.hs @@ -77,13 +77,13 @@ perform file key = do breakhardlink obj = whenM (catchBoolIO $ (> 1) . linkCount <$> liftIO (R.getFileStatus obj)) $ do mfc <- withTSDelta (liftIO . genInodeCache file) unlessM (sameInodeCache obj (maybeToList mfc)) $ do - modifyContent obj $ replaceGitAnnexDirFile (fromRawFilePath obj) $ \tmp -> do + modifyContentDir obj $ replaceGitAnnexDirFile (fromRawFilePath obj) $ \tmp -> do unlessM (checkedCopyFile key obj (toRawFilePath tmp) Nothing) $ giveup "unable to lock file" Database.Keys.storeInodeCaches key [obj] -- Try to repopulate obj from an unmodified associated file. - repopulate obj = modifyContent obj $ do + repopulate obj = modifyContentDir obj $ do g <- Annex.gitRepo fs <- map (`fromTopFilePath` g) <$> Database.Keys.getAssociatedFiles key diff --git a/Upgrade/V5/Direct.hs b/Upgrade/V5/Direct.hs index 9903f84d07..49665e8773 100644 --- a/Upgrade/V5/Direct.hs +++ b/Upgrade/V5/Direct.hs @@ -97,7 +97,7 @@ associatedFilesRelative key = do removeAssociatedFiles :: Key -> Annex () removeAssociatedFiles key = do mapping <- calcRepo $ gitAnnexMapping key - modifyContent mapping $ + modifyContentDir mapping $ liftIO $ removeWhenExistsWith R.removeLink mapping {- Checks if a file in the tree, associated with a key, has not been modified. @@ -124,7 +124,7 @@ recordedInodeCache key = withInodeCacheFile key $ \f -> {- Removes an inode cache. -} removeInodeCache :: Key -> Annex () removeInodeCache key = withInodeCacheFile key $ \f -> - modifyContent f $ + modifyContentDir f $ liftIO $ removeWhenExistsWith R.removeLink f withInodeCacheFile :: Key -> (RawFilePath -> Annex a) -> Annex a diff --git a/doc/todo/command_to___34__migrate__34___from_adjusted_mode/comment_6_e8ab74bb4951fe7f0c71630bb278ad6f._comment b/doc/todo/command_to___34__migrate__34___from_adjusted_mode/comment_6_e8ab74bb4951fe7f0c71630bb278ad6f._comment new file mode 100644 index 0000000000..3b17573088 --- /dev/null +++ b/doc/todo/command_to___34__migrate__34___from_adjusted_mode/comment_6_e8ab74bb4951fe7f0c71630bb278ad6f._comment @@ -0,0 +1,26 @@ +[[!comment format=mdwn + username="joey" + subject="""comment 6""" + date="2022-05-16T15:24:43Z" + content=""" +If fsck locks the content for removal, then moves it to the preferred +location, how is that any different from git-annex first dropping content +and then very quickly retrieving another copy and storing it in the other +location? The only difference is timing, but things like being suspended +and resumed can affect timing. + +So, if there is a problem with fsck doing that, there would also be a more +general problem, that could occur in other circumstances, even if only +rarely. + +One way to see the general problem happen would be to have two processes +trying to drop the same object. One process finds the object location, then +stalls. Meanwhile, the second process drops the object. Then the first +process resumes, and locks for removal. Per comment #5 this will result in +a dangling lock file in the object directory. I have not managed to get +this to happen yet though. + +A fix for the general problem is to make it not create the +object directory when opening the object lock file. So I've made that +change. +"""]]