Re: [PATCH 8/9] xfs: introduce xfs_buf_submit[_wait]

[Dave Chinner <david@fromorbit.com>]

On Fri, Aug 15, 2014 at 06:10:20AM -0700, Christoph Hellwig wrote:
> > diff --git a/fs/xfs/xfs_bmap_util.c b/fs/xfs/xfs_bmap_util.c
> > index 2f1e30d..c53cc03 100644
> > --- a/fs/xfs/xfs_bmap_util.c
> > +++ b/fs/xfs/xfs_bmap_util.c
> > @@ -1157,12 +1157,7 @@ xfs_zero_remaining_bytes(
> 
> xfs_zero_remaining_bytes really should be switched to
> xfs_buf_read_uncached + xfs_bwrite instead of all this mess just to
> micro-optimize a few memory allocation away that don't even happen for
> the common case of blocksize == PAGE_SIZE.  I'm not even sure we
> should be using the buffer cache at all for something inside a regular
> file, but that's a discussion for another time.

xfs_zero_remaining_bytes is uses an uncached buffer, so we're not
using the buffer cache at all for the blocks being zeroed. That is
why it does the flag twiddling dance it does. However, consolidation
all the different block zeroing functions we have is an exercise for
a different day....

> 
> >  void
> > +xfs_buf_submit(
> > +	struct xfs_buf	*bp)
> >  {
> >  	trace_xfs_buf_iorequest(bp, _RET_IP_);
> 
> I suspect these two should have properly name and separate trace
> points now?

Yes. Just haven't got to it.

> It also seems like a lot of the guts of the two functions are
> still the same, so factoring that into a __xfs_buf_submit helper
> would be useful.

Possibly, if we can ensure that the helper it never called directly
by any other code. Then we end up back in the mess we are currently
in.

> > +	 * If _xfs_buf_ioapply failed, 
> > +	 * we can get back here with only the IO
> >  	 * reference we took above.  _xfs_buf_ioend will drop it to zero, so
> >  	 * we'd better run completion processing synchronously so that the we
> > +	 * don't return to the caller with completion still pending.
> > +	 * this allows the caller to check b_error safely without
> > +	 * waiting
> >  	 */
> >  	if (atomic_dec_and_test(&bp->b_io_remaining) == 1) {
> >  		if (bp->b_error || !(bp->b_flags & XBF_ASYNC))
> 
> I don't think the !ASYNC case can happen here, can it?

Right, I forget to clean that part up when I was splitting up the
functions.

> 
> > +		if (!wait)
> >  			list_del_init(&bp->b_list);
> > +		else
> > +			xfs_buf_hold(bp);
> 
> Maybe switch this around to avoid the negated condition in the if else?
> 
> Also might this be worth a change of it's own?

Yeah, that's probably a good idea - the algorithm change is not
directly related to the interface change.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs