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@xxxxxxxxxxx
http://oss.sgi.com/mailman/listinfo/xfs
