xfs
[Top] [All Lists]

Re: [PATCH 4/7] xfs: split direct IO write path from xfs_file_aio_write

To: Christoph Hellwig <hch@xxxxxxxxxxxxx>
Subject: Re: [PATCH 4/7] xfs: split direct IO write path from xfs_file_aio_write
From: Dave Chinner <david@xxxxxxxxxxxxx>
Date: Fri, 17 Dec 2010 18:31:25 +1100
Cc: xfs@xxxxxxxxxxx
In-reply-to: <20101216120629.GC20445@xxxxxxxxxxxxx>
References: <1292376208-16282-1-git-send-email-david@xxxxxxxxxxxxx> <1292376208-16282-5-git-send-email-david@xxxxxxxxxxxxx> <20101216120629.GC20445@xxxxxxxxxxxxx>
User-agent: Mutt/1.5.20 (2009-06-14)
On Thu, Dec 16, 2010 at 07:06:29AM -0500, Christoph Hellwig wrote:
> > +STATIC ssize_t
> > +xfs_file_dio_aio_write(
> > +   struct kiocb            *iocb,
> > +   const struct iovec      *iovp,
> > +   unsigned long           nr_segs,
> > +   loff_t                  pos,
> > +   size_t                  ocount,
> > +   int                     *need_i_mutex,
> > +   int                     *iolock)
> 
> need_i_mutex == 1 is equivalent to iolock == XFS_IOLOCK_EXCL, I think
> it's a good idea to throw in a patch to remove the need_i_mutex variable
> before this patch.
> 
> Maybe that patch can even add xfs_rw_lock or similar helpers to
> manipulate the i_mutex, the iolock, and the iolock variable together?

That sounds like a fine idea.

> Speaking of that, shouldn't xfs_file_aio_read also take the iolock
> exclusive during the page invalidation and then demote it, just like
> the write case?  The above helpers would enforce that nicely.

Probably, though it might be best to leave that to another cleanup
series. I'll see how much perturbation of the read path it makes....

> > +   if ((pos & target->bt_smask) || (count & target->bt_smask))
> > +           return XFS_ERROR(-EINVAL);
> 
> For the error traps to work this needs to be
> 
>       -XFS_ERROR(EINVAL);

Ok, will fix. I just copied the existing code.

Cheers,

Dave.
-- 
Dave Chinner
david@xxxxxxxxxxxxx

<Prev in Thread] Current Thread [Next in Thread>