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

[Christoph Hellwig <hch@xxxxxxxxxxxxx>]

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.