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 at lst.de>
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 at fromorbit.com