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