On Mon, Jan 11, 2021 at 08:14:12PM +0100, Christoph Hellwig wrote:
> On Mon, Jan 11, 2021 at 01:59:20PM -0500, Brian Foster wrote:
> > > + /*
> > > + * Bmap information not read in yet or no blocks allocated at all?
> > > + */
> > > + if (!(ifp->if_flags & XFS_IFEXTENTS) || !ip->i_d.di_nblocks)
> > > + return 0;
> > > +
> > > + ret = xfs_ilock_iocb(iocb, XFS_ILOCK_SHARED);
> > > + if (ret)
> > > + return ret;
> >
> > It looks like this helper is only called with ILOCK_SHARED already held.
>
> xfs_dio_write_exclusive is called with the iolock held shared, but not
> the ilock.
>
Oops, thinko.
>
> > > + if (iocb->ki_pos > i_size_read(inode)) {
> > > + if (iocb->ki_flags & IOCB_NOWAIT)
> > > return -EAGAIN;
> >
> > Not sure why we need this check here if we'll eventually fall into the
> > serialized check. It seems safer to me to just do 'iolock =
> > XFS_IOLOCK_EXCL;' here and carry on.
>
> It seems a little pointless to first acquire the lock for that. But
> in the end this is not what the patch is about, so I'm happy to drop it
> if that is preferred.
>
It may be fine, but I think the codepath is easier to grok without
creating duplicate/racy branches for logic that potentially impacts
behavior (whereas the lock upgrade only impacts performance).
Particularly in this case where if NOWAIT is set, we'll already bail at
the first appropriate point anyways.
Brian
> > > - if (unaligned_io) {
> > > + if (exclusive_io) {
> >
> > Hmm.. so if we hold or upgrade to ILOCK_EXCL from the start for whatever
> > reason, we'd never actually check whether the I/O is "exclusive" or not.
> > Then we fall into here, demote the lock and the iomap layer may very
> > well end up doing subblock zeroing. I suspect if we wanted to maintain
> > this logic, the exclusive I/O check should occur for any subblock_io
> > regardless of how the lock is held.
>
> True.
>
