Re: [PATCH 12/18] xfs: introduce the xfs_iext_cursor abstraction

[Brian Foster <bfoster@redhat.com>]

On Thu, Nov 02, 2017 at 07:51:38PM +0100, Christoph Hellwig wrote:
> On Thu, Nov 02, 2017 at 01:14:37PM -0400, Brian Foster wrote:
> > Can we just open code ext->idx here rather than maintain two counters,
> > or will that go away later?
> 
> This area will change quite a bit.  Please take a look at the end result.
> 

Ok.. after a quick look at the final xfs_iread_extents(), it appears
that concern is no longer relevant.

> > > -	ASSERT(bma->idx >= 0);
> > > -	ASSERT(bma->idx <= xfs_iext_count(ifp));
> > 
> > I think it might be useful to encapsulate this check (which is also part
> > of xfs_iext_get_extent()) into a cursor validation helper so we can
> > preserve these asserts (here and in the other bmap functions). E.g.,
> > something like:
> > 
> > 	ASSERT(xfs_iext_valid(&bma->ext));
> 
> I'll take a look at that.
> 
> > 1.) Why do we place these new cursors directly on the stack as opposed
> > to dynamic allocation?
> 
> Why would be do a dynamic allocation?
> 

I just noticed it as a divergence from our other cursor implementations.
This is of course a different implementation, however. I guess it more
depends on what ends up in the cursor. It looks like the cursor ends up
with an integer index and leaf pointer, the latter of which appears to
be memory owned by the inode..? If that's the case, then perhaps there
is really no point to dynamic allocation of the cursor itself.

> > 2.) Why not encode the fork/inode in the cursor rather than passing them
> > around throughout the helper functions?
> 
> We could do that, but I'm not sure it's really worth the effort.

I'm not against cleaning it up after the fact. The broader question was
whether there was any reason we'd need to pass the fork to every call.
If not, I think combining it with the cursor may provide a cleaner
interface.

Brian

> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html