From 3e2f1f73cbc5fc10475745b3c3133267bd1850a7 Mon Sep 17 00:00:00 2001 From: Joey Hess Date: Mon, 21 Mar 2022 13:12:02 -0400 Subject: [PATCH] add back inode to directory special remote ContentIdentifier Directory special remotes with importtree=yes have changed to once more take inodes into account. This will cause extra work when importing from a directory on a FAT filesystem that changes inodes on every mount. To avoid that extra work, set ignoreinodes=yes when initializing a new directory special remote, or change the configuration of your existing remote: git-annex enableremote foo ignoreinodes=yes This will mean a one-time re-import of all contents from every directory special remote due to the changed setting. 73df633a6215faa093e4a7524c6d328aa988aed1 thought it was too unlikely that there would be modifications that the inode number was needed to notice. That was probably right; it's very unlikely that a file will get modified and end up with the same size and mtime as before. But, what was not considered is that a program like NextCloud might write two files with different content so closely together that they share the mtime. The inode is necessary to detect that situation. Sponsored-by: Max Thoursie on Patreon --- CHANGELOG | 7 ++ Remote/Directory.hs | 94 ++++++++++--------- ...ee_replaces_files_with_identical_size.mdwn | 2 + ..._2742c2cbde12aa85b8254dd6907fcfc1._comment | 36 +++++++ ..._46336dc995a7cefb8ebbfe98f098e258._comment | 15 +++ doc/special_remotes/directory.mdwn | 8 ++ 6 files changed, 117 insertions(+), 45 deletions(-) create mode 100644 doc/bugs/importtree_replaces_files_with_identical_size/comment_1_2742c2cbde12aa85b8254dd6907fcfc1._comment create mode 100644 doc/bugs/importtree_replaces_files_with_identical_size/comment_2_46336dc995a7cefb8ebbfe98f098e258._comment diff --git a/CHANGELOG b/CHANGELOG index d82817ed83..428ff3e6ed 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -21,6 +21,13 @@ git-annex (10.20220223) UNRELEASED; urgency=medium Thanks, sternenseemann for the patch. * test: Runs tests in parallel to speed up the test suite. * test: Added --jobs option. + * Directory special remotes with importtree=yes have changed to once more + take inodes into account. This will cause extra work when importing + from a directory on a FAT filesystem that changes inodes on every + mount. To avoid that extra work, set ignoreinodes=yes when initializing + a new directory special remote, or change the configuration of your + existing remote: + git-annex enableremote foo ignoreinodes=yes -- Joey Hess Wed, 23 Feb 2022 14:14:09 -0400 diff --git a/Remote/Directory.hs b/Remote/Directory.hs index 4650f054d3..93a78eda14 100644 --- a/Remote/Directory.hs +++ b/Remote/Directory.hs @@ -1,6 +1,6 @@ {- A "remote" that is just a filesystem directory. - - - Copyright 2011-2021 Joey Hess + - Copyright 2011-2022 Joey Hess - - Licensed under the GNU AGPL version 3 or higher. -} @@ -54,6 +54,8 @@ remote = specialRemoteType $ RemoteType , configParser = mkRemoteConfigParser [ optionalStringParser directoryField (FieldDesc "(required) where the special remote stores data") + , yesNoParser ignoreinodesField (Just False) + (FieldDesc "ignore inodes when importing/exporting") ] , setup = directorySetup , exportSupported = exportIsSupported @@ -64,12 +66,17 @@ remote = specialRemoteType $ RemoteType directoryField :: RemoteConfigField directoryField = Accepted "directory" +ignoreinodesField :: RemoteConfigField +ignoreinodesField = Accepted "ignoreinodes" + gen :: Git.Repo -> UUID -> RemoteConfig -> RemoteGitConfig -> RemoteStateHandle -> Annex (Maybe Remote) gen r u rc gc rs = do c <- parsedRemoteConfig remote rc cst <- remoteCost gc cheapRemoteCost let chunkconfig = getChunkConfig c cow <- liftIO newCopyCoWTried + let ii = IgnoreInodes $ fromMaybe True $ + getRemoteConfigValue ignoreinodesField c return $ Just $ specialRemote c (storeKeyM dir chunkconfig cow) (retrieveKeyFileM dir chunkconfig cow) @@ -99,15 +106,15 @@ gen r u rc gc rs = do , renameExport = renameExportM dir } , importActions = ImportActions - { listImportableContents = listImportableContentsM dir - , importKey = Just (importKeyM dir) - , retrieveExportWithContentIdentifier = retrieveExportWithContentIdentifierM dir cow - , storeExportWithContentIdentifier = storeExportWithContentIdentifierM dir cow - , removeExportWithContentIdentifier = removeExportWithContentIdentifierM dir + { listImportableContents = listImportableContentsM ii dir + , importKey = Just (importKeyM ii dir) + , retrieveExportWithContentIdentifier = retrieveExportWithContentIdentifierM ii dir cow + , storeExportWithContentIdentifier = storeExportWithContentIdentifierM ii dir cow + , removeExportWithContentIdentifier = removeExportWithContentIdentifierM ii dir -- Not needed because removeExportWithContentIdentifier -- auto-removes empty directories. , removeExportDirectoryWhenEmpty = Nothing - , checkPresentExportWithContentIdentifier = checkPresentExportWithContentIdentifierM dir + , checkPresentExportWithContentIdentifier = checkPresentExportWithContentIdentifierM ii dir } , whereisKey = Nothing , remoteFsck = Nothing @@ -351,8 +358,8 @@ removeExportLocation topdir loc = mkExportLocation loc' in go (upFrom loc') =<< tryIO (removeDirectory p) -listImportableContentsM :: RawFilePath -> Annex (Maybe (ImportableContentsChunkable Annex (ContentIdentifier, ByteSize))) -listImportableContentsM dir = liftIO $ do +listImportableContentsM :: IgnoreInodes -> RawFilePath -> Annex (Maybe (ImportableContentsChunkable Annex (ContentIdentifier, ByteSize))) +listImportableContentsM ii dir = liftIO $ do l <- dirContentsRecursive (fromRawFilePath dir) l' <- mapM (go . toRawFilePath) l return $ Just $ ImportableContentsComplete $ @@ -360,41 +367,38 @@ listImportableContentsM dir = liftIO $ do where go f = do st <- R.getFileStatus f - mkContentIdentifier f st >>= \case + mkContentIdentifier ii f st >>= \case Nothing -> return Nothing Just cid -> do relf <- relPathDirToFile dir f sz <- getFileSize' f st return $ Just (mkImportLocation relf, (cid, sz)) --- Make a ContentIdentifier that contains the size and mtime of the file. --- If the file is not a regular file, this will return Nothing. --- --- The inode is zeroed because often this is used for import from a --- FAT filesystem, whose inodes change each time it's mounted, and --- including inodes would cause repeated re-hashing of files, and --- bloat the git-annex branch with changes to content identifier logs. +newtype IgnoreInodes = IgnoreInodes Bool + +-- Make a ContentIdentifier that contains the size and mtime of the file, +-- and also normally the inode, unless ignoreinodes=yes. -- --- This does mean that swaps of two files with the same size and --- mtime won't be noticed, nor will modifications to files that --- preserve the size and mtime. Both very unlikely so acceptable. -mkContentIdentifier :: RawFilePath -> FileStatus -> IO (Maybe ContentIdentifier) -mkContentIdentifier f st = - fmap (ContentIdentifier . encodeBS . showInodeCache) - <$> toInodeCache' noTSDelta f st 0 +-- If the file is not a regular file, this will return Nothing. +mkContentIdentifier :: IgnoreInodes -> RawFilePath -> FileStatus -> IO (Maybe ContentIdentifier) +mkContentIdentifier (IgnoreInodes ii) f st = + liftIO $ fmap (ContentIdentifier . encodeBS . showInodeCache) + <$> if ii + then toInodeCache' noTSDelta f st 0 + else toInodeCache noTSDelta f st guardSameContentIdentifiers :: a -> ContentIdentifier -> Maybe ContentIdentifier -> a guardSameContentIdentifiers cont old new | new == Just old = cont | otherwise = giveup "file content has changed" -importKeyM :: RawFilePath -> ExportLocation -> ContentIdentifier -> ByteSize -> MeterUpdate -> Annex (Maybe Key) -importKeyM dir loc cid sz p = do +importKeyM :: IgnoreInodes -> RawFilePath -> ExportLocation -> ContentIdentifier -> ByteSize -> MeterUpdate -> Annex (Maybe Key) +importKeyM ii dir loc cid sz p = do backend <- chooseBackend f unsizedk <- fst <$> genKey ks p backend let k = alterKey unsizedk $ \kd -> kd { keySize = keySize kd <|> Just sz } - currcid <- liftIO $ mkContentIdentifier absf + currcid <- liftIO $ mkContentIdentifier ii absf =<< R.getFileStatus absf guardSameContentIdentifiers (return (Just k)) cid currcid where @@ -406,8 +410,8 @@ importKeyM dir loc cid sz p = do , inodeCache = Nothing } -retrieveExportWithContentIdentifierM :: RawFilePath -> CopyCoWTried -> ExportLocation -> ContentIdentifier -> FilePath -> Annex Key -> MeterUpdate -> Annex Key -retrieveExportWithContentIdentifierM dir cow loc cid dest mkkey p = +retrieveExportWithContentIdentifierM :: IgnoreInodes -> RawFilePath -> CopyCoWTried -> ExportLocation -> ContentIdentifier -> FilePath -> Annex Key -> MeterUpdate -> Annex Key +retrieveExportWithContentIdentifierM ii dir cow loc cid dest mkkey p = precheck docopy where f = exportPath dir loc @@ -449,7 +453,7 @@ retrieveExportWithContentIdentifierM dir cow loc cid dest mkkey p = -- Check before copy, to avoid expensive copy of wrong file -- content. precheck cont = guardSameContentIdentifiers cont cid - =<< liftIO . mkContentIdentifier f + =<< liftIO . mkContentIdentifier ii f =<< liftIO (R.getFileStatus f) -- Check after copy, in case the file was changed while it was @@ -470,7 +474,7 @@ retrieveExportWithContentIdentifierM dir cow loc cid dest mkkey p = #else postchecknoncow cont = do #endif - currcid <- liftIO $ mkContentIdentifier f + currcid <- liftIO $ mkContentIdentifier ii f #ifndef mingw32_HOST_OS =<< getFdStatus fd #else @@ -484,22 +488,22 @@ retrieveExportWithContentIdentifierM dir cow loc cid dest mkkey p = -- the modified version was copied CoW, and then the file was -- restored to the original content before this check. postcheckcow cont = do - currcid <- liftIO $ mkContentIdentifier f + currcid <- liftIO $ mkContentIdentifier ii f =<< R.getFileStatus f guardSameContentIdentifiers cont cid currcid -storeExportWithContentIdentifierM :: RawFilePath -> CopyCoWTried -> FilePath -> Key -> ExportLocation -> [ContentIdentifier] -> MeterUpdate -> Annex ContentIdentifier -storeExportWithContentIdentifierM dir cow src _k loc overwritablecids p = do +storeExportWithContentIdentifierM :: IgnoreInodes -> RawFilePath -> CopyCoWTried -> FilePath -> Key -> ExportLocation -> [ContentIdentifier] -> MeterUpdate -> Annex ContentIdentifier +storeExportWithContentIdentifierM ii dir cow src _k loc overwritablecids p = do liftIO $ createDirectoryUnder dir (toRawFilePath destdir) withTmpFileIn destdir template $ \tmpf tmph -> do liftIO $ hClose tmph void $ fileCopier cow src tmpf p Nothing let tmpf' = toRawFilePath tmpf resetAnnexFilePerm tmpf' - liftIO (getFileStatus tmpf) >>= liftIO . mkContentIdentifier tmpf' >>= \case + liftIO (getFileStatus tmpf) >>= liftIO . mkContentIdentifier ii tmpf' >>= \case Nothing -> giveup "unable to generate content identifier" Just newcid -> do - checkExportContent dir loc + checkExportContent ii dir loc overwritablecids (giveup "unsafe to overwrite file") (const $ liftIO $ rename tmpf dest) @@ -509,16 +513,16 @@ storeExportWithContentIdentifierM dir cow src _k loc overwritablecids p = do (destdir, base) = splitFileName dest template = relatedTemplate (base ++ ".tmp") -removeExportWithContentIdentifierM :: RawFilePath -> Key -> ExportLocation -> [ContentIdentifier] -> Annex () -removeExportWithContentIdentifierM dir k loc removeablecids = - checkExportContent dir loc removeablecids (giveup "unsafe to remove modified file") $ \case +removeExportWithContentIdentifierM :: IgnoreInodes -> RawFilePath -> Key -> ExportLocation -> [ContentIdentifier] -> Annex () +removeExportWithContentIdentifierM ii dir k loc removeablecids = + checkExportContent ii dir loc removeablecids (giveup "unsafe to remove modified file") $ \case DoesNotExist -> return () KnownContentIdentifier -> removeExportM dir k loc -checkPresentExportWithContentIdentifierM :: RawFilePath -> Key -> ExportLocation -> [ContentIdentifier] -> Annex Bool -checkPresentExportWithContentIdentifierM dir _k loc knowncids = +checkPresentExportWithContentIdentifierM :: IgnoreInodes -> RawFilePath -> Key -> ExportLocation -> [ContentIdentifier] -> Annex Bool +checkPresentExportWithContentIdentifierM ii dir _k loc knowncids = checkPresentGeneric' dir $ - checkExportContent dir loc knowncids (return False) $ \case + checkExportContent ii dir loc knowncids (return False) $ \case DoesNotExist -> return False KnownContentIdentifier -> return True @@ -539,12 +543,12 @@ data CheckResult = DoesNotExist | KnownContentIdentifier -- -- So, it suffices to check if the destination file's current -- content is known, and immediately run the callback. -checkExportContent :: RawFilePath -> ExportLocation -> [ContentIdentifier] -> Annex a -> (CheckResult -> Annex a) -> Annex a -checkExportContent dir loc knowncids unsafe callback = +checkExportContent :: IgnoreInodes -> RawFilePath -> ExportLocation -> [ContentIdentifier] -> Annex a -> (CheckResult -> Annex a) -> Annex a +checkExportContent ii dir loc knowncids unsafe callback = tryWhenExists (liftIO $ R.getFileStatus dest) >>= \case Just destst | not (isRegularFile destst) -> unsafe - | otherwise -> catchDefaultIO Nothing (liftIO $ mkContentIdentifier dest destst) >>= \case + | otherwise -> catchDefaultIO Nothing (liftIO $ mkContentIdentifier ii dest destst) >>= \case Just destcid | destcid `elem` knowncids -> callback KnownContentIdentifier -- dest exists with other content diff --git a/doc/bugs/importtree_replaces_files_with_identical_size.mdwn b/doc/bugs/importtree_replaces_files_with_identical_size.mdwn index 6f07aeed52..bf0b78cd29 100644 --- a/doc/bugs/importtree_replaces_files_with_identical_size.mdwn +++ b/doc/bugs/importtree_replaces_files_with_identical_size.mdwn @@ -61,3 +61,5 @@ Unfortunately, I don't have the output of git-annex anymore. However, the used c ### Have you had any luck using git-annex before? (Sometimes we get tired of reading bug reports all day and a lil' positive end note does wonders) I've been successfully using git-annex to manage the growing collection of photos I take for several years now. Synchronizing the collection across several disks while files are renamed and some disks are only rarely synchronized and some contain only a subset of the photos wouldn't have been possible without git-annex. Also, using git-annex gives me the safety that the original photos will never be modified. There is nothing better than using git-annex to verify that all photos have been copied unmodified from a memory card before wiping it. + +> [[fixed|done]] --[[Joey]] diff --git a/doc/bugs/importtree_replaces_files_with_identical_size/comment_1_2742c2cbde12aa85b8254dd6907fcfc1._comment b/doc/bugs/importtree_replaces_files_with_identical_size/comment_1_2742c2cbde12aa85b8254dd6907fcfc1._comment new file mode 100644 index 0000000000..b2637e54cc --- /dev/null +++ b/doc/bugs/importtree_replaces_files_with_identical_size/comment_1_2742c2cbde12aa85b8254dd6907fcfc1._comment @@ -0,0 +1,36 @@ +[[!comment format=mdwn + username="joey" + subject="""comment 1""" + date="2022-03-21T16:20:27Z" + content=""" +The directory special remote use the size and mtime as the +ContentIdentifier of a file. It used to use inode number as well, but +some filesystems make up new inodes on each mount so that was dropped. + +So when two files have the same size and mtime, it will import one of them, +and then when it comes to the other one, see that it already has that +ContentIdentifier and use the content it already imported. + +For example: + + joey@darkstar:/tmp/t>git-annex initremote d type=directory directory=../d importtree=yes exporttree=yes encryption=none + joey@darkstar:/tmp/t>echo hi > ../d/1 + joey@darkstar:/tmp/t>echo no > ../d/2 + joey@darkstar:/tmp/t>touch --reference=../d/1 ../d/2 + joey@darkstar:/tmp/t>git-annex import master --from d + joey@darkstar:/tmp/t>git merge d/master + joey@darkstar:/tmp/t>cmp 1 2 + joey@darkstar:/tmp/t> + +Perhaps NextCloud is for some reason giving multiple files the same mtime? + +This is rather unlikely to happen naturally, especially on modern +filesystems that have high resolution mtimes. Even "echo 1> 1; echo 2>2" +generates files with 2 different mtimes. To get the same mtime, the two +files have to be written by the same process at very close to the same +time. + +Probably that's why NextCloud managed to get the same mtime, although why +it would be rewriting the content of the file when the content had not +changed I don't know and perhaps that's a bug of some kind in it. +"""]] diff --git a/doc/bugs/importtree_replaces_files_with_identical_size/comment_2_46336dc995a7cefb8ebbfe98f098e258._comment b/doc/bugs/importtree_replaces_files_with_identical_size/comment_2_46336dc995a7cefb8ebbfe98f098e258._comment new file mode 100644 index 0000000000..3ce0061a02 --- /dev/null +++ b/doc/bugs/importtree_replaces_files_with_identical_size/comment_2_46336dc995a7cefb8ebbfe98f098e258._comment @@ -0,0 +1,15 @@ +[[!comment format=mdwn + username="joey" + subject="""comment 2""" + date="2022-03-21T16:38:43Z" + content=""" +So this makes me think that [[!73df633a6215faa093e4a7524c6d328aa988aed1]] +needs to be reverted and the inode number used again. But that did fix a +bug of a sort, that importing from a fat filesystem after it was remounted +would re-import everything. So I suppose the best that can be done is to +make it configurable whether inodes should be ignored or not when importing +from a directory special remote. And presumably default to not ignoring +them. + +done +"""]] diff --git a/doc/special_remotes/directory.mdwn b/doc/special_remotes/directory.mdwn index a15a84823e..5fc99961eb 100644 --- a/doc/special_remotes/directory.mdwn +++ b/doc/special_remotes/directory.mdwn @@ -39,6 +39,14 @@ remote: by [[git-annex-import]]. It will not be usable as a general-purpose special remote. +* `ignoreinodes` - Usually when importing, the inode numbers + of files are used to detect when files have changed. Since some + filesystems generate new inode numbers each time they are mounted, + that can lead to extra work being done. Setting this to "yes" will + ignore the inode numbers and so avoid that extra work. + This should not be used when the filesystem has stable inode numbers, + as it does risk confusing two files that have the same size and mtime. + Setup example: # git annex initremote usbdrive type=directory directory=/media/usbdrive/ encryption=none -- 2.30.2