Re: [PATCH RFC] generic/730: ensure EIO after device delete

From: Boris Burkov <boris@bur.io>
To: Filipe Manana <fdmanana@kernel.org>
Cc: "Darrick J. Wong" <djwong@kernel.org>,
	kernel-team@fb.com, fstests@vger.kernel.org,
	Christoph Hellwig <hch@lst.de>
Subject: Re: [PATCH RFC] generic/730: ensure EIO after device delete
Date: Sat, 16 Mar 2024 07:40:08 -0700	[thread overview]
Message-ID: <20240316144008.GA3615790@zen.localdomain> (raw)
In-Reply-To: <CAL3q7H71Roae5kqc0Hh2=CZxCfLd9Kp7bqAfgo5OcUz==opVow@mail.gmail.com>

On Sat, Mar 16, 2024 at 01:08:57PM +0000, Filipe Manana wrote:
> On Fri, Mar 15, 2024 at 11:30 PM Boris Burkov <boris@bur.io> wrote:
> >
> > On Thu, Mar 14, 2024 at 07:56:28PM -0700, Darrick J. Wong wrote:
> > > On Wed, Mar 13, 2024 at 04:47:47PM -0700, Boris Burkov wrote:
> > > > This test removes a SCSI debug device out from under a mounted
> > > > filesystem with a (probably) dirty file. This assumes that page cache
> > > > cannot save us from EIO, for a reason that I can't quite explain. In
> > > > fact, this test fails for exactly that reason, at least on btrfs.
> > > >
> > > > The original patches:
> > > >
> > > >       https://lore.kernel.org/fstests/20230807112100.GB15405@lst.de/
> > > >
> > > > refer to this passing on xfs and not btrfs, so I suspect I am missing
> > > > something. With that said, on my machine this actually fails on xfs with
> > > > and without my patch, so this is clearly not enough.
> > > >
> > > > High level, I am trying to understand what is really the expected
> > > > behavior from a filesystem under this condition and what this test is
> > > > getting at. Of btrfs, ext4, and xfs, only ext4 passes it, while btrfs
> > > > does pass with this additional syncing/cache dropping to nudge it to an
> > > > error.
> > >
> > > Does btrfs prefetch pagecache data as soon as a file opens?  Or I guess
> > > it could be that xfs trips an IO error and shuts down, and
> > > xfs_file_read_iter will return EIO after that happens.
> > >
> > > --D
> > >
> >
> > I might be wildly misunderstanding, since I'm very far from an expert on
> > the page cache, but didn't we just do a buffered write to the file? So it
> > would make sense for it to be in cache?
> >
> > At any rate, what I observed on my system was that xfs does not pass
> > this test. Curious if that is only on my system or just the current
> > state of the test.
> 
> Instead of xfs, do you mean btrfs?
> 
> For me xfs and ext4 always pass this test, i.e. the cat command fails
> with -EIO, both with and without your patch.
> 
> On the other hand, btrfs and f2fs always fail without your patch, and
> pass with your patch applied.

$ sudo ./check generic/730
FSTYP         -- xfs (non-debug)
PLATFORM      -- Linux/x86_64 v0 6.8.0-rc7+ #205 SMP PREEMPT_DYNAMIC Thu Mar 14 16:44:32 PDT 2024
MKFS_OPTIONS  -- -f /dev/mapper/tst-scr0
MOUNT_OPTIONS -- /dev/mapper/tst-scr0 /mnt/scratch

generic/730 1s ... [failed, exit status 1]- output mismatch (see /home/vmuser/fstests/results//generic/730.out.bad)
    --- tests/generic/730.out   2023-11-01 22:14:50.011000000 +0000
    +++ /home/vmuser/fstests/results//generic/730.out.bad       2024-03-16 14:38:02.431000000 +0000
    @@ -1,2 +1 @@
     QA output created by 730
    -cat: -: Input/output error
    ...
    (Run 'diff -u /home/vmuser/fstests/tests/generic/730.out /home/vmuser/fstests/results//generic/730.out.bad'  to see the entire diff)
Ran: generic/730
Failures: generic/730
Failed 1 of 1 tests

I suspect it has something to do with my Kconfig. Perhaps "xfs (non-debug)" is a clue?

> 
> From a btrfs point of view, it makes sense for the cat command to
> succeed, since all the file data is in the page cache.

That was my intuition as well, as I mentioned above.

> 
> I don't know why the cat command fails (-EIO) with xfs and ext4, and I
> haven't looked at any of their source code (my knowledge of those
> filesystem's internals is too little anyway).
> 
> An alternative to your patch is just to make the write with direct IO
> by passing -d to $XFS_IO_PROG. It makes the test succeed on these 4
> filesystems.
> 
> 

I like this idea a lot more than my patch!

> >
> > Summary of my test matrix:
> > ext4: passes with and without this patch
> > btrfs: fails without this patch, passes with it
> > xfs: fails with and without this patch
> >
> > For that reason, I don't think this a good patch, I just mostly want to
> > figure out where to go with this test!
> >
> > Thanks,
> > Boris
> >
> > > > Signed-off-by: Boris Burkov <boris@bur.io>
> > > > ---
> > > >  tests/generic/730 | 3 +++
> > > >  1 file changed, 3 insertions(+)
> > > >
> > > > diff --git a/tests/generic/730 b/tests/generic/730
> > > > index 11308cdaa..ca5037c57 100755
> > > > --- a/tests/generic/730
> > > > +++ b/tests/generic/730
> > > > @@ -47,6 +47,9 @@ exec 3< $SCSI_DEBUG_MNT/testfile
> > > >  # delete the scsi debug device while it still has dirty data
> > > >  echo 1 > /sys/block/$(_short_dev $SCSI_DEBUG_DEV)/device/delete
> > > >
> > > > +sync
> > > > +echo 3 > /proc/sys/vm/drop_caches
> > > > +
> > > >  # try to read from the file, which should give us -EIO
> > > >  cat <&3 > /dev/null
> > > >
> > > > --
> > > > 2.43.0
> > > >
> > > >
> >