xfs
[Top] [All Lists]

Re: [PATCH 2/4] [PATCH 2/4] xfs: remove wrappers for read/write file ope

To: Christoph Hellwig <hch@xxxxxxxxxxxxx>
Subject: Re: [PATCH 2/4] [PATCH 2/4] xfs: remove wrappers for read/write file operations
From: Dave Chinner <david@xxxxxxxxxxxxx>
Date: Wed, 17 Feb 2010 14:55:11 +1100
Cc: xfs@xxxxxxxxxxx
In-reply-to: <20100215094604.318261333@xxxxxxxxxxxxxxxxxxxxxx>
References: <20100215094445.528696829@xxxxxxxxxxxxxxxxxxxxxx> <20100215094604.318261333@xxxxxxxxxxxxxxxxxxxxxx>
User-agent: Mutt/1.5.18 (2008-05-17)
On Mon, Feb 15, 2010 at 04:44:47AM -0500, Christoph Hellwig wrote:
> Currently the aio_read, aio_write, splice_read and splice_write file
> operations are divided into a low-level routine doing all the work and
> one that implements the Linux file operations and does minimal argument
> wrapping.  This is a leftover from the days of the vnode operations layer
> and can be removed to simplify the code a lot.
> 
> Signed-off-by: Christoph Hellwig <hch@xxxxxx>

One main issue I see:

....

> -ssize_t                              /* bytes written, or (-) error */
> -xfs_write(
> -     struct xfs_inode        *xip,
> +STATIC ssize_t
> +xfs_file_aio_write(
>       struct kiocb            *iocb,
>       const struct iovec      *iovp,
> -     unsigned int            nsegs,
> -     loff_t                  *offset,
> -     int                     ioflags)
> +     unsigned long           nr_segs,
> +     loff_t                  pos)
>  {
>       struct file             *file = iocb->ki_filp;
>       struct address_space    *mapping = file->f_mapping;
>       struct inode            *inode = mapping->host;
> -     unsigned long           segs = nsegs;
> -     xfs_mount_t             *mp;
> +     struct xfs_inode        *ip = XFS_I(inode);
> +     struct xfs_mount        *mp = ip->i_mount;
>       ssize_t                 ret = 0, error = 0;
> +     int                     ioflags = 0;
>       xfs_fsize_t             isize, new_size;
>       int                     iolock;
>       int                     eventsent = 0;
>       size_t                  ocount = 0, count;
> -     loff_t                  pos;
>       int                     need_i_mutex;
>  
>       XFS_STATS_INC(xs_write_calls);
>  
> -     error = generic_segment_checks(iovp, &segs, &ocount, VERIFY_READ);
> +     BUG_ON(iocb->ki_pos != pos);

You've changed a local variable "pos" which had the value
iocb->ki_pos to a function parameter of the same name which has a
different value.  Given this, all the existing uses of "pos" in this
function need to be converted to "iocb->ki_pos" as the old
xfs_write() never saw the original "pos" variable passed to
xfs_file_aio_write().

.....

> @@ -535,30 +550,30 @@ relock:
>               mutex_lock(&inode->i_mutex);
>       }
>  
> -     xfs_ilock(xip, XFS_ILOCK_EXCL|iolock);
> +     xfs_ilock(ip, XFS_ILOCK_EXCL|iolock);
>  
>  start:
>       error = -generic_write_checks(file, &pos, &count,
>                                       S_ISBLK(inode->i_mode));

Such as here. Actually, I'm surprised the compiler let you take
the address of a function parameter considering parameters may be
passed in registers....

>       if (error) {
> -             xfs_iunlock(xip, XFS_ILOCK_EXCL|iolock);
> +             xfs_iunlock(ip, XFS_ILOCK_EXCL|iolock);
>               goto out_unlock_mutex;
>       }
>  
> -     if ((DM_EVENT_ENABLED(xip, DM_EVENT_WRITE) &&
> +     if ((DM_EVENT_ENABLED(ip, DM_EVENT_WRITE) &&
>           !(ioflags & IO_INVIS) && !eventsent)) {
>               int             dmflags = FILP_DELAY_FLAG(file);
>  
>               if (need_i_mutex)
>                       dmflags |= DM_FLAGS_IMUX;
>  
> -             xfs_iunlock(xip, XFS_ILOCK_EXCL);
> -             error = XFS_SEND_DATA(xip->i_mount, DM_EVENT_WRITE, xip,
> +             xfs_iunlock(ip, XFS_ILOCK_EXCL);
> +             error = XFS_SEND_DATA(ip->i_mount, DM_EVENT_WRITE, ip,
>                                     pos, count, dmflags, &iolock);

And here. There's lots of others that haven't been converted - can
you make sure you get them all?

> @@ -642,16 +657,16 @@ start:
>  
>               if (need_i_mutex) {
>                       /* demote the lock now the cached pages are gone */
> -                     xfs_ilock_demote(xip, XFS_IOLOCK_EXCL);
> +                     xfs_ilock_demote(ip, XFS_IOLOCK_EXCL);
>                       mutex_unlock(&inode->i_mutex);
>  
>                       iolock = XFS_IOLOCK_SHARED;
>                       need_i_mutex = 0;
>               }
>  
> -             trace_xfs_file_direct_write(xip, count, *offset, ioflags);
> +             trace_xfs_file_direct_write(ip, count, iocb->ki_pos, ioflags);
>               ret = generic_file_direct_write(iocb, iovp,
> -                             &segs, pos, offset, count, ocount);
> +                             &nr_segs, pos, &iocb->ki_pos, count, ocount);

But you did convert some ;)

Cheers,

Dave.
-- 
Dave Chinner
david@xxxxxxxxxxxxx

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