Re: [PATCH RFC 1/4] xfs: clean up cow fork reservation and tag inodes correctly

[Brian Foster <bfoster@redhat.com>]

On Fri, Nov 18, 2016 at 12:11:46AM -0800, Christoph Hellwig wrote:
> On Tue, Nov 15, 2016 at 01:11:01PM -0500, Brian Foster wrote:
> > On Tue, Nov 15, 2016 at 06:16:21AM -0800, Christoph Hellwig wrote:
> > > > +	if (imap->br_startoff != got.br_startoff ||
> > > > +	    imap->br_blockcount != got.br_blockcount)
> > > >  		xfs_inode_set_cowblocks_tag(ip);
> > > 
> > > Can't got.br_blockcount be smaller than imap->br_blockcount if we have
> > > an existing COW fork reservation lying around behind the whole we're
> > > filling?  Also they way xfs_bmapi_reserve_delalloc works the startoff
> > > will be the same.  E.g. this check should probably be:
> > > 
> > 
> > Good point, though I think it can be smaller or larger without
> > necessarily having preallocation due to being merged with surrounding
> > extents. I'm not quite sure what the right answer for that is with
> > regard to tagging, but I do think it's better to have false positive
> > tagging than false negatives.
> 
> Good point, merging can change both the start and length.  Based on
> that I think tagging in the caller of xfs_bmapi_reserve_delalloc is
> wrong, and we need to do it inside xfs_bmapi_reserve_delalloc where
> we know if we did speculative preallocation.

xfs_bmapi_reserve_delalloc() doesn't really know if it's doing
preallocation outside of the extent size hint alignment. I don't think
we want to try and bury all of the prealloc stuff down in the bmapi
code, though we may be able to add a prealloc parameter to
bmapi_reserve_delalloc() such that it can add the prealloc itself and
also tell the difference between prealloc and merge cases. I'll play
around with it, thanks.

Brian