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

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

[Boris Burkov]

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.

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

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

[Darrick J. Wong]

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

> 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
> 
> 

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

[Boris Burkov]

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.

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
> > 
> > 

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

[Filipe Manana]

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.

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.

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.


>
> 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
> > >
> > >
>

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

[Boris Burkov]

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
> > > >
> > > >
> >

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

[Filipe Manana]

On Sat, Mar 16, 2024 at 2:39 PM Boris Burkov <boris@bur.io> wrote:
>
> 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?

Mine is also non-debug and yet it always succeeds for me (without your
patch or any other of course):

root 17:40:02 /home/fdmanana/git/hub/xfstests > ./check generic/730
FSTYP         -- xfs (non-debug)
PLATFORM      -- Linux/x86_64 debian0 6.8.0-rc7-btrfs-next-153+ #1 SMP
PREEMPT_DYNAMIC Mon Mar  4 17:19:19 WET 2024
MKFS_OPTIONS  -- -f /dev/sdb
MOUNT_OPTIONS -- /dev/sdb /home/fdmanana/btrfs-tests/scratch_1

generic/730 2s ...  2s
Ran: generic/730
Passed all 1 tests


>
> >
> > 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
> > > > >
> > > > >
> > >

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

[Christoph Hellwig]

On Sat, Mar 16, 2024 at 05:42:30PM +0000, Filipe Manana wrote:
> Mine is also non-debug and yet it always succeeds for me (without your
> patch or any other of course):

Same here.

And the idea behind the test is that when the device is removed the
file system should always return an error, i.e. have a check for shutdown
fs in the read path.