On Mon, May 02, 2022 at 02:21:17PM -0700, Stefan Roesch wrote:
>
>
> On 4/28/22 2:54 PM, Dave Chinner wrote:
> > 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@xxxxxx>
> >>>> ---
> >>>> 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?
> >
>
> In the function xfs_file_buffered_write() the code calls the function
> xfs_ilock(). The xfs_inode pointer that is passed in is iocb->ki_filp->f_mapping->host.
> The one used in xfs_ilock_iocb is ki_filp->f_inode.
>
> After getting the lock, the code in xfs_file_buffered_write calls the
> function xfs_buffered_write_iomap_begin(). In this function the code
> calls xfs_ilock() for ki_filp->f_inode in exclusive mode.
>
> If I replace the first xfs_ilock() call with xfs_ilock_iocb(), then it looks
> like I get a deadlock.
>
> Am I missing something?
Yes. They take different locks. xfs_file_buffered_write() takes the
IOLOCK, xfs_buffered_write_iomap_begin() takes the ILOCK....
> I can:
> - replace the pointer to iocb with pointer to xfs_inode in the function xfs_ilock_iocb()
> and also pass in the flags value as a parameter.
> or
> - create function xfs_ilock_inode(), which xfs_ilock_iocb() calls. The existing
> calls will not need to change, only the xfs_ilock in xfs_file_buffered_write()
> will use xfs_ilock_inode().
You're making this way more complex than it needs to be. As I said:
> > Regardless, if this is a problem, then just pass the XFS inode to
> > xfs_ilock_iocb() and this is a moot point.
Cheers,
Dave.
--
Dave Chinner
david@xxxxxxxxxxxxx
