Re: [PATCH] Introduce SEEK_DATA/SEEK_HOLE support to XFS V4

[Christoph Hellwig <hch@infradead.org>]

Hi Jeff,

thanks a lot for the patch, it looks good to except for some more
nitpicks around the unwritten extent probing.

The other issue is the patch description format - the version changelog
should go below the  --- line.

> +	do {
> +		unsigned int	i;
> +		unsigned	nr_pages;
> +		int		want = min_t(pgoff_t, end - index,
> +					     (pgoff_t)PAGEVEC_SIZE - 1) + 1;
> +		nr_pages = pagevec_lookup_tag(&pvec, inode->i_mapping,
> +					      &index, tag, want);
> +		if (nr_pages == 0) {
> +			/*
> +			 * Try to lookup pages in writeback mode from the
> +			 * beginning if no more dirty page can be probed.
> +			 */
> +probe_done:
> +			if (tag == PAGECACHE_TAG_DIRTY) {
> +				tag = PAGECACHE_TAG_WRITEBACK;
> +				goto again;
> +			}
> +			break;

The code flow here looks very confusing.  Why not pass the tag as an
argument to the function, then calling it twice and use the minimum?
(that probably also wants a helper instead of duplication)


> +				 * dirty data in the page cache it can be
> +				 * identified by having BH_Unwritten set in
> +				 * each buffer. Also, the buffer head state
> +				 * might be in BH_Uptodate if the buffer
> +				 * writeback procedure was fired, we need to
> +				 * examine it too.
> +				 */
> +				if (buffer_unwritten(bh) ||
> +				    buffer_uptodate(bh)) {
> +					found = true;
> +					if (get_offset)
> +						*offset = XFS_FSB_TO_B(
> +								mp, last);

Currently seek hole doesn't set get_offset we skip the whole extent.
This seems a bit inconsistent - shouldn't we also return that offset
for the hole case? if the dirty data only starts past the start block
of the map the first blocks of it still are a hole.

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