Re: [PATCH] generic: add test for fsync after shrinking truncate and rename

[Jayashree Mohan <jayashree2912@gmail.com>]

Hi Amir,

> I went back to look at similar fsync tests by Filipe:
> generic/{106,107,335,336,341,342,343,348,498,501,502,509,510,512}
>
> I found some alleged subtle mistakes about SOMC assumptions.
>
> generic/336 does:
> touch $SCRATCH_MNT/a/foo
> ln $SCRATCH_MNT/a/foo $SCRATCH_MNT/b/foo_link
> touch $SCRATCH_MNT/b/bar
> sync
> unlink $SCRATCH_MNT/b/foo_link
> mv $SCRATCH_MNT/b/bar $SCRATCH_MNT/c/
> $XFS_IO_PROG -c "fsync" $SCRATCH_MNT/a/foo

This is probably what's happening in this particular test :

SOMC requires:
          fsync(a/foo) must ensure unlink(b/foo_link)  (because they
were linked at some point)

But what happens is:
           fsync(a/foo)          -->  unlink(b/foo_link)
           unlink(b/foo_link)  -->  fsync(b)
           fsync(b)                 -->  rename goes through

SOMC should only require that the unlink persists. The rename
operation persists due to the side-effect of SOMC. So we should be
only testing if the unlink operation went through.

Take a look at this thread that describes the bug which resulted in
this test case (https://patchwork.kernel.org/patch/8293181/).
The file bar was lost during the rename operation and I think this
test simply wanted to verify that file bar was intact in either one of
the directories. Probably the reason this test was written was not to
test SOMC, but to verify that the log replay in btrfs does not delete
the renamed file - luckily it happens to pass on other file systems
because they all persist the rename operation as a side effect of
SOMC. To me, this test currently checks for something more than SOMC.
The check must be modified to only test if b/foo_link is removed, and
that the file bar is *either* in directory b or c.

Test 343 is a similar case where in btrfs log replay ended up
persisting the file in both the source and destination directories
after a rename operation
(https://patchwork.kernel.org/patch/8766401/). The checks must be
updated here as well.

Thanks,
Jayashree