On Tue, Jan 12, 2021 at 11:42:57AM +0100, Christoph Hellwig wrote:
> > diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
> > index bba33be17eff..f5c75404b8a5 100644
> > --- a/fs/xfs/xfs_file.c
> > +++ b/fs/xfs/xfs_file.c
> > @@ -408,7 +408,7 @@ xfs_file_aio_write_checks(
> > drained_dio = true;
> > goto restart;
> > }
> > -
> > +
>
> Spurious unrelated whitespace change.
>
> > struct iomap_dio_rw_args args = {
> > .iocb = iocb,
> > .iter = from,
> > .ops = &xfs_direct_write_iomap_ops,
> > .dops = &xfs_dio_write_ops,
> > .wait_for_completion = is_sync_kiocb(iocb),
> > - .nonblocking = (iocb->ki_flags & IOCB_NOWAIT),
> > + .nonblocking = true,
>
> I think this is in many ways wrong. As far as I can tell you want this
> so that we get the imap_spans_range in xfs_direct_write_iomap_begin. But
> we should not trigger any of the other checks, so we'd really need
> another flag instead of reusing this one.
>
It's really the br_state != XFS_EXT_NORM check that we want for the
unaligned case, isn't it?
> imap_spans_range is a bit pessimistic for avoiding the exclusive lock,
> but I guess we could live that if it is clearly documented as helping
> with the implementation, but we really should not automatically trigger
> all the other effects of nowait I/O.
>
Regardless, I agree on this point. I don't have a strong opinion in
general on this approach vs. the other, but it does seem odd to me to
overload the broader nowait semantics with the unaligned I/O checks. I
see that it works for the primary case we care about, but this also
means things like the _has_page() check now trigger exclusivity for the
unaligned case where that doesn't seem to be necessary. I do like the
previous cleanups so I suspect if we worked this into a new
'subblock_io' flag that indicates to the lower layer whether the
filesystem can allow zeroing, that might clean much of this up.
Brian
