Re: [PATCH v2 2/2] nilfs2: switch O_DIRECT to iomap based operations

Hi Ryusuke,

On Sun, 2026-08-09 at 08:51 +0900, Ryusuke Konishi wrote:
> Hi Viacheslav,
> 
> On Sat, Aug 8, 2026 at 9:35 AM Viacheslav Dubeyko wrote:
> ...
> > +static ssize_t nilfs_file_read_iter(struct kiocb *iocb, struct
> > iov_iter *to)
> > +{
> > +       if (iocb->ki_flags & IOCB_DIRECT) {
> > +               return iomap_dio_rw(iocb, to, &nilfs_iomap_ops,
> > +                                   NULL, 0, NULL, 0);
> > +       } else
> > +               return generic_file_read_iter(iocb, to);
> > +}
> 
> Among the tests that used to pass before the iomap conversion, I
> found that
> running xfstests generic/418 and generic/465 now fails with a "broken
> bmap"
> error, causing the filesystem to remount read-only.
> 
> This happens because iomap_dio_rw() is called without holding the
> shared
> inode lock.  To prevent this race, I think iomap_dio_rw() should be
> protected as follows:
> 
>         struct inode *inode = file_inode(iocb->ki_filp);
>         ssize_t ret;
> 
>         if (iocb->ki_flags & IOCB_DIRECT) {
>                 inode_lock_shared(inode);
>                 ret = iomap_dio_rw(iocb, to, &nilfs_iomap_ops,
>                                 NULL, 0, NULL, 0);
>                 inode_unlock_shared(inode);
>         } else {
>                 ret = generic_file_read_iter(iocb, to);
>         }
> 
>         return ret;
> 
> Adding this locking resolved the failures in both generic/418 and
> generic/465 in my tests.

Yeah, makes sense. Let double check this.

> 
> > +
> > +static ssize_t nilfs_file_write_iter(struct kiocb *iocb, struct
> > iov_iter *from)
> > +{
> > +       /*
> > +        * NILFS2 cannot perform true direct I/O writes: new blocks
> > are
> > +        * delay-allocated and are only given a real disk address
> > when
> > +        * the segment constructor writes them out as part of a
> > log,
> > +        * which works directly on buffer_head lists rather than
> > +        * through iomap. Fall back to the ordinary buffered write
> > path
> > +        * for O_DIRECT writes.
> > +        */
> 
> Also, regarding this comment, following Christoph's suggestion, how
> about simplifying it as follows?
> 
>         /*
>          * NILFS2 lacks direct I/O write support; fall back to
> buffered writes.
>          */

Sorry, my bad. I missed this. Let me rework the comment.

Thanks,
Slava.