Re: [PATCH 2/3] xfs: don't release bios on completion immediately

[Brian Foster <bfoster@redhat.com>]

On Fri, Mar 11, 2016 at 03:47:58PM +0100, Christoph Hellwig wrote:
> On Thu, Mar 03, 2016 at 10:17:22AM -0500, Brian Foster wrote:
> > > +xfs_finish_page_writeback(
> > > +	struct page	*page,
> > > +	unsigned int	start,
> > > +	unsigned int	end,
> > > +	int		error)
> > > +{
> > > +	struct buffer_head	*head, *bh;
> > > +	unsigned int		off = 0;
> > > +
> > > +	bh = head = page_buffers(page);
> > > +
> > > +	do {
> > > +		if (start > off)
> > > +			goto next_bh;
> > 
> > Probably not an issue for current usage, which appears to be on buffer
> > size granularity, but shouldn't this check whether start is beyond the
> > end of the current buffer (e.g., start >= off + bh->b_size)?
> 
> I don't understand that question.  We get called for a given page,
> and a start and end offset inside that page.  These offsets by design
> need to be aligned to the filesystem blocksize.  So what we basically
> want is to skip a few buffers at the beginning and/or end of the page,
> and the code seems to handle that fine.
> 

Yes, that's what I meant by it not being an issue for current usage.

My point was that when looking at the new helper, nothing explicitly
indicates the offsets must be block aligned. This page buffer_head
walking loop is rewritten all over the fs/ code in a manner that is
correct for arbitrary offsets, so this instance looked wrong at first
glance. Then I realized it probably works fine once looking at the
callers.

I'm fine with it as is if we add an assert for the alignment of the
offsets.

Brian

> > >  	mempool_free(ioend, xfs_ioend_pool);
> > >  }
> > >  
> > > +
> > 
> > Unnecessary whitespace here.
> 
> Fixed.
> 
> _______________________________________________
> xfs mailing list
> xfs@oss.sgi.com
> http://oss.sgi.com/mailman/listinfo/xfs

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