[Top] [All Lists]

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

To: Dave Chinner <david@xxxxxxxxxxxxx>
Subject: Re: [PATCH 2/4] [PATCH 2/4] xfs: remove wrappers for read/write file operations
From: Christoph Hellwig <hch@xxxxxxxxxxxxx>
Date: Wed, 17 Feb 2010 03:31:06 -0500
Cc: Christoph Hellwig <hch@xxxxxxxxxxxxx>, xfs@xxxxxxxxxxx
In-reply-to: <20100217035511.GJ28392@xxxxxxxxxxxxxxxx>
References: <20100215094445.528696829@xxxxxxxxxxxxxxxxxxxxxx> <20100215094604.318261333@xxxxxxxxxxxxxxxxxxxxxx> <20100217035511.GJ28392@xxxxxxxxxxxxxxxx>
User-agent: Mutt/1.5.19 (2009-01-05)
On Wed, Feb 17, 2010 at 02:55:11PM +1100, Dave Chinner wrote:
> 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().

Oh, I should have explained this in more detail.  The aio_read/aio_write
ABI both has a pos argument and the file position in iocb->ki_pos.
They were added for allowing aio that does partial I/O in each method
call using retries, but we don't actually use the anywhere.  Thus these
two are always these same and we even enforce that with a

        BUG_ON(iocb->ki_pos != pos);

in the code (both old and new).  Now how does the old pos local variable
come in play?  The old code didn't want to pass the kiocb to the
low-level xfs-write function, but as want the offset it passes a pointer
to iocb->ki_pos, which is called offset.  We take a local copy of it
before we might start modifying it, which we call pos.  pos gets updated
early in generic_write_checks if this is an O_APPEND write, but
otherwise stays immutable and marks the position where this write
started, while iocb->ki_pos (aka the old offset) gets updated by
generic_file_direct_write / generic_file_buffered_write to the new
file position after the I/O was done.

> 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....

Taking the address of arguments is perfectly valid in C, the only thing
you can't take addresses of are "register" variables.  This code is
the same as mm/filemap.c:__generic_file_aio_write, btw.

> > -           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 ;)

I did carefull convert all offset references to ki->ki_pos, but all
uses of pos stay the same.

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