2020-02-05 09:14:54

by Murphy Zhou

[permalink] [raw]
Subject: ext4 dio RWF_NOWAIT change

Hi,

Kernel commit 378f32bab3714f04c4e0c3aee4129f6703805550
Author: Matthew Bobrowski <[email protected]>
Date: Tue Nov 5 23:02:39 2019 +1100

ext4: introduce direct I/O write using iomap infrastructure


Changed the logic of dio+RWF_NOWAIT

from:

- if (!inode_trylock(inode)) {
- if (iocb->ki_flags & IOCB_NOWAIT)
- return -EAGAIN;
- inode_lock(inode);
- }


to:

+ if (iocb->ki_flags & IOCB_NOWAIT) {
+ if (!inode_trylock(inode))
+ return -EAGAIN;
+ } else {
+ inode_lock(inode);
+ }


fstests generic/471 expecet EAGAIN on this situation, so it started to
fail since than.

The current logic is similar with other filesystems, but only ext4 fails
on geneirc/471.

Any thoughts how to fix this?

Thanks,
Murphy


2020-02-07 15:38:49

by Theodore Ts'o

[permalink] [raw]
Subject: Re: ext4 dio RWF_NOWAIT change

On Wed, Feb 05, 2020 at 05:13:44PM +0800, Murphy Zhou wrote:
> Hi,
>
> Kernel commit 378f32bab3714f04c4e0c3aee4129f6703805550
> Author: Matthew Bobrowski <[email protected]>
> Date: Tue Nov 5 23:02:39 2019 +1100
>
> ext4: introduce direct I/O write using iomap infrastructure
>
>
> Changed the logic of dio+RWF_NOWAIT
>
> from:
>
> - if (!inode_trylock(inode)) {
> - if (iocb->ki_flags & IOCB_NOWAIT)
> - return -EAGAIN;
> - inode_lock(inode);
> - }
>
>
> to:
>
> + if (iocb->ki_flags & IOCB_NOWAIT) {
> + if (!inode_trylock(inode))
> + return -EAGAIN;
> + } else {
> + inode_lock(inode);
> + }
>
>
> fstests generic/471 expecet EAGAIN on this situation, so it started to
> fail since than.

I don't understand why this specific change would cause the situation.
In the generic/471 test, here iocb->ki_flags will have IOCB_NOWAIT
set, and in that case I don't see how there would be a change in
behavior with respect to EAGAIN being returned.

In any case, I've been suppressing generic/471 because of concerns
that the test is bogus so I hadn't noticed. From
kvm-xfstests/test-appliance/files/root/fs/global_exclude:

# The test generic/471 tests the RWF_NOWAIT flag; however
# how it is supposed to work with file systems is disputed,
# and not all device drivers support it. As a result
# it doesn't work if an LVM device driver is in use (as is the
# case with gce-xfstests). So let's suppress it for now. For
# more details see:
# https://lore.kernel.org/linux-block/[email protected]/
generic/471

- Ted

2020-02-12 10:50:54

by Murphy Zhou

[permalink] [raw]
Subject: Re: ext4 dio RWF_NOWAIT change

On Fri, Feb 07, 2020 at 10:38:24AM -0500, Theodore Y. Ts'o wrote:
> On Wed, Feb 05, 2020 at 05:13:44PM +0800, Murphy Zhou wrote:
> > Hi,
> >
> > Kernel commit 378f32bab3714f04c4e0c3aee4129f6703805550
> > Author: Matthew Bobrowski <[email protected]>
> > Date: Tue Nov 5 23:02:39 2019 +1100
> >
> > ext4: introduce direct I/O write using iomap infrastructure
> >
> >
> > Changed the logic of dio+RWF_NOWAIT
> >
> > from:
> >
> > - if (!inode_trylock(inode)) {
> > - if (iocb->ki_flags & IOCB_NOWAIT)
> > - return -EAGAIN;
> > - inode_lock(inode);
> > - }
> >
> >
> > to:
> >
> > + if (iocb->ki_flags & IOCB_NOWAIT) {
> > + if (!inode_trylock(inode))
> > + return -EAGAIN;
> > + } else {
> > + inode_lock(inode);
> > + }
> >
> >
> > fstests generic/471 expecet EAGAIN on this situation, so it started to
> > fail since than.
>
> I don't understand why this specific change would cause the situation.
> In the generic/471 test, here iocb->ki_flags will have IOCB_NOWAIT
> set, and in that case I don't see how there would be a change in
> behavior with respect to EAGAIN being returned.
>
> In any case, I've been suppressing generic/471 because of concerns
> that the test is bogus so I hadn't noticed. From
> kvm-xfstests/test-appliance/files/root/fs/global_exclude:
>
> # The test generic/471 tests the RWF_NOWAIT flag; however
> # how it is supposed to work with file systems is disputed,
> # and not all device drivers support it. As a result
> # it doesn't work if an LVM device driver is in use (as is the
> # case with gce-xfstests). So let's suppress it for now. For
> # more details see:
> # https://lore.kernel.org/linux-block/[email protected]/
> generic/471
>
> - Ted

Thanks for all the details! Ted.

Murphy