Re: [RFC PATCH] xfs: refactor dir2 leaf readahead shadow buffer cleverness

["Darrick J. Wong" <darrick.wong@oracle.com>]

On Thu, Apr 27, 2017 at 12:32:28AM -0700, Christoph Hellwig wrote:
> On Mon, Apr 24, 2017 at 07:03:53PM -0700, Darrick J. Wong wrote:
> > Currently, the dir2 leaf block getdents function uses a complex state
> > tracking mechanism to create a shadow copy of the block mappings and
> > then uses the shadow copy to schedule readahead.  Since the read and
> > readahead functions are perfectly capable of reading the mappings
> > themselves, we can tear all that out in favor of a simpler function that
> > simply keeps pushing the readahead window further out.
> 
> I like where this goes a lot.  A few more comments below:
> 
> > +	/* Flush old buf; remember its daddr for error detection. */
> > +	if (*bpp) {
> > +		old_daddr = (*bpp)->b_bn;
> > +		xfs_trans_brelse(args->trans, *bpp);
> 
> I don't really understand the whole old_daddr logic.  How could we
> go backwards in the logic block space?

I was being overly paranoid that we could somehow end up with the same
buffer, but I've convinced myself that this isn't actually possible.

> > +	 * Look for mapped directory blocks at or above the current
> > +	 * offset.  We must truncate down to the nearest directory
> > +	 * block to start the scanning operation.
> 
> Use all 80 chars available on the terminal for comments, please :)

Ok.

> > +	last_da = xfs_dir2_byte_to_da(geo, XFS_DIR2_LEAF_OFFSET);
> > +	map_off = xfs_dir2_db_to_da(geo, xfs_dir2_byte_to_db(geo, *cur_off));
> > +	do {
> > +		nmap = 1;
> > +		error = xfs_bmapi_read(dp, map_off, last_da - map_off,
> > +				&map, &nmap, 0);
> > +		if (error || !nmap)
> > +			goto out;
> > +		map_off = map.br_startoff + map.br_blockcount;
> > +	} while (map_off < last_da && map.br_startblock == HOLESTARTBLOCK);
> > +
> > +	if (map.br_startblock == HOLESTARTBLOCK)
> >  		goto out;
> 
> This shows that xfs_bmapi_read really is the wrong interface for the
> calles.  Raw calls to xfs_iext_lookup_extent / xfs_iext_get_extent would
> make this loop easier to understand, and also more efficient by not
> doing the full lookup.
> 
> > +	while (ra_want > 0 && next_ra < last_da) {
> > +		nmap = 1;
> > +		error = xfs_bmapi_read(dp, next_ra, last_da - next_ra,
> > +				&map, &nmap, 0);
> > +		if (error || !nmap)
> > +			break;
> > +		next_ra = roundup((xfs_dablk_t)map.br_startoff, geo->fsbcount);
> > +		while (ra_want > 0 &&
> > +		       map.br_startblock != HOLESTARTBLOCK &&
> > +		       next_ra < map.br_startoff + map.br_blockcount) {
> > +			xfs_dir3_data_readahead(dp, next_ra, -2);
> > +			*ra_blk = next_ra;
> > +			ra_want -= geo->fsbcount;
> > +			next_ra += geo->fsbcount;
> >  		}
> > +		next_ra = map.br_startoff + map.br_blockcount;
> 
> Same here..

<nod>

--D