Re: [RFC PATCH v1 11/18] xfs: add async buffered write support

[Dave Chinner <david@fromorbit.com>]

On Thu, Apr 28, 2022 at 12:58:59PM -0700, Stefan Roesch wrote:
> 
> 
> On 4/26/22 3:56 PM, Dave Chinner wrote:
> > On Tue, Apr 26, 2022 at 10:43:28AM -0700, Stefan Roesch wrote:
> >> This adds the async buffered write support to XFS. For async buffered
> >> write requests, the request will return -EAGAIN if the ilock cannot be
> >> obtained immediately.
> >>
> >> Signed-off-by: Stefan Roesch <shr@fb.com>
> >> ---
> >>  fs/xfs/xfs_file.c | 10 ++++++----
> >>  1 file changed, 6 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
> >> index 6f9da1059e8b..49d54b939502 100644
> >> --- a/fs/xfs/xfs_file.c
> >> +++ b/fs/xfs/xfs_file.c
> >> @@ -739,12 +739,14 @@ xfs_file_buffered_write(
> >>  	bool			cleared_space = false;
> >>  	int			iolock;
> >>  
> >> -	if (iocb->ki_flags & IOCB_NOWAIT)
> >> -		return -EOPNOTSUPP;
> >> -
> >>  write_retry:
> >>  	iolock = XFS_IOLOCK_EXCL;
> >> -	xfs_ilock(ip, iolock);
> >> +	if (iocb->ki_flags & IOCB_NOWAIT) {
> >> +		if (!xfs_ilock_nowait(ip, iolock))
> >> +			return -EAGAIN;
> >> +	} else {
> >> +		xfs_ilock(ip, iolock);
> >> +	}
> > 
> > xfs_ilock_iocb().
> > 
> 
> The helper xfs_ilock_iocb cannot be used as it hardcoded to use iocb->ki_filp to
> get a pointer to the xfs_inode.

And the problem with that is?

I mean, look at what xfs_file_buffered_write() does to get the
xfs_inode 10 lines about that change:

xfs_file_buffered_write(
        struct kiocb            *iocb,
        struct iov_iter         *from)
{
        struct file             *file = iocb->ki_filp;
        struct address_space    *mapping = file->f_mapping;
        struct inode            *inode = mapping->host;
        struct xfs_inode        *ip = XFS_I(inode);

In what cases does file_inode(iocb->ki_filp) point to a different
inode than iocb->ki_filp->f_mapping->host? The dio write path assumes
that file_inode(iocb->ki_filp) is correct, as do both the buffered
and dio read paths.

What makes the buffered write path special in that
file_inode(iocb->ki_filp) is not correctly set whilst
iocb->ki_filp->f_mapping->host is?

Regardless, if this is a problem, then just pass the XFS inode to
xfs_ilock_iocb() and this is a moot point.

> However here we need to use iocb->ki_filp->f_mapping->host.
> I'll split off new helper for this in the next version of the patch.

We don't need a new helper here, either.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com