Re: [PATCH v6 2/4] xfs: Introduce a new function to find the desired type of offset from page cache

[Mark Tinguely <tinguely@sgi.com>]

On 08/03/12 01:10, Jeff Liu wrote:
> Introduce helpers to probe data or hole offset from page cache for unwritten extents.
>
> ---
>   fs/xfs/xfs_file.c |  213 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>   1 files changed, 213 insertions(+), 0 deletions(-)
>
> diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
> index 98642cf..aff0c30 100644
> --- a/fs/xfs/xfs_file.c
> +++ b/fs/xfs/xfs_file.c
> @@ -36,6 +36,7 @@
>


> + * keep the offset argument unchanged.
> + */
> +STATIC bool
> +xfs_lookup_buffer_offset(

I am fine with this helper routine.


> +
> +/*
> + * This routine is called to find out and return a data or hole offset
> + * from the page cache for unwritten extents according to the desired
> + * type for xfs_seek_data() or xfs_seek_hole().
> + *
> + * The argument offset is used to tell where we start to search from the
> + * page cache.  Map is used to figure out the end points of the range to
> + * lookup pages.
> + *
> + * Return true if the desired type of offset was found, and the argument
> + * offset is filled with that address.  Otherwise, return false and keep
> + * offset unchanged.
> + */
> +STATIC bool
> +xfs_find_get_desired_pgoff(
> +	struct inode		*inode,
> +	struct xfs_bmbt_irec	*map,
> +	unsigned int		type,
> +	loff_t			*offset)
> +{
> +	struct xfs_inode	*ip = XFS_I(inode);
> +	struct xfs_mount	*mp = ip->i_mount;
> +	struct pagevec		pvec;
> +	pgoff_t			index;
> +	pgoff_t			end;
> +	loff_t			endoff;
> +	loff_t			startoff = *offset;
> +	loff_t			lastoff = startoff;
> +	bool			found = false;
> +
> +	pagevec_init(&pvec, 0);
> +
> +	index = startoff>>  PAGE_CACHE_SHIFT;
> +	endoff = XFS_FSB_TO_B(mp, map->br_startoff + map->br_blockcount);
> +	end = endoff>>  PAGE_CACHE_SHIFT;
> +	do {
> +		int		want;
> +		unsigned	nr_pages;
> +		unsigned int	i;
> +
> +		want = min_t(pgoff_t, end - index, (pgoff_t)PAGEVEC_SIZE);
> +		nr_pages = pagevec_lookup(&pvec, inode->i_mapping, index,
> +					  want);
> +		/*
> +		 * No page mapped into given range.  If we are searching holes
> +		 * and if this is the first time we got into the loop, it means
> +		 * that the given offset is landed in a hole, return it.
> +		 *
> +		 * If we have already stepped through some block buffers to find
> +		 * holes but they all contains data.  In this case, the last
> +		 * offset is already updated and pointed to the end of the last
> +		 * mapped page, if it does not reach the endpoint to search,
> +		 * that means there should be a hole between them.
> +		 */
> +		if (nr_pages == 0) {
> +			/* Data search found nothing */
> +			if (type == DATA_OFF)
> +				break;
> +
> +			ASSERT(type == HOLE_OFF);
> +			if (lastoff == startoff || lastoff<  endoff) {
> +				found = true;
> +				*offset = lastoff;
> +			}
> +			break;
> +		}
> +
> +		/*
> +		 * At lease we found one page.  If this is the first time we
> +		 * step into the loop, and if the first page index offset is
> +		 * greater than the given search offset, a hole was found.
> +		 */
> +		if (type == HOLE_OFF&&  lastoff == startoff&&
> +		    lastoff<  page_offset(pvec.pages[0])) {
> +			found = true;
> +			break;
> +		}

I played with the code and appreciate the trickiness of the startoff.
I this can be refined a bit more. see below.

> +
> +		for (i = 0; i<  nr_pages; i++) {
> +			struct page	*page = pvec.pages[i];
> +			loff_t		b_offset;
> +
> +			/*
> +			 * Page index is out of range, searching done.
> +			 * If the current offset is not reaches the end
> +			 * of the specified search range, there should
> +			 * be a hole between them.
> +			 */
> +			if (page->index>  end) {
> +				if (type == HOLE_OFF&&  lastoff<  endoff) {
> +					*offset = lastoff;
> +					found = true;
> +				}
> +				goto out;
> +			}

Before we take the lock, we can check to see if the page starts later 
than expected (a hole). The pre-loop start check can be removed and 
something like the follow added:

			/* Skip over this page if it precedes the start offset */
                         if (page_offset(page) + PAGE_SIZE < startoff)
                                 continue;

                         if (type == HOLE_OFF && page_offset(page) > 
lastoff) {
                                 *offset = lastoff;
                                 found = true;
                                 goto out;
                         }

My experiment passes your newest, more extensive tests. I can give you the
complete experiment patch if you want.


> +
> +			lock_page(page);
> +			/*
> +			 * Page truncated or invalidated(page->mapping == NULL).
> +			 * We can freely skip it and proceed to check the next
> +			 * page.
> +			 */
> +			if (unlikely(page->mapping != inode->i_mapping)) {
> +				unlock_page(page);
> +				continue;
> +			}
> +
> +			if (!page_has_buffers(page)) {
> +				unlock_page(page);
> +				continue;
> +			}

Would this be a hole condition? I suppose the next iteration of the loop 
will figure this out.

> +
> +			found = xfs_lookup_buffer_offset(page,&b_offset, type);
> +			if (found) {
> +				/*
> +				 * The found offset may be less than the start
> +				 * point to search if this is the first time to
> +				 * come here.
> +				 */
> +				*offset = max_t(loff_t, startoff, b_offset);
> +				goto out;
> +			}
> +
> +			/*
> +			 * We either searching data but nothing was found, or
> +			 * searching hole but found a data buffer.  In either
> +			 * case, probably the next page contains the desired
> +			 * things, update the last offset to it so.
> +			 */
> +			lastoff = page_offset(page) + PAGE_SIZE;
> +			unlock_page(page);
> +		}
> +
> +		/*
> +		 * The number of returned pages less than our desired, search
> +		 * done.  In this case, nothing was found for searching data,
> +		 * but we found a hole behind the last offset.
> +		 */
> +		if (nr_pages<  want) {
> +			if (type == HOLE_OFF) {
> +				*offset = lastoff;
> +				found = true;
> +			}
> +			break;
> +		}
> +
> +		index = pvec.pages[i - 1]->index + 1;
> +		pagevec_release(&pvec);
> +	} while (index<  end);
> +
> +out:
> +	pagevec_release(&pvec);
> +	return found;
> +}
> +
>   STATIC loff_t
>   xfs_seek_data(
>   	struct file		*file,

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