On Tue, 28 Feb 2012 20:17:28 +0800, Hillf Danton <dhillf@xxxxxxxxx> wrote:
> On Tue, Feb 28, 2012 at 6:15 PM, Aneesh Kumar K.V
> <aneesh.kumar@xxxxxxxxxxxxxxxxxx> wrote:
> >
> > Will update the patch with these details
> >
>
> A scratch is cooked, based on the -next tree, for accelerating your redelivery,
> if you like it, in which i_mutex is eliminated directly and page lock is used.
>
> -hd
This looks much better than what I had.
>
>
> --- a/fs/hugetlbfs/inode.c Tue Feb 28 19:43:32 2012
> +++ b/fs/hugetlbfs/inode.c Tue Feb 28 19:56:50 2012
> @@ -245,17 +245,10 @@ static ssize_t hugetlbfs_read(struct fil
> loff_t isize;
> ssize_t retval = 0;
>
> - mutex_lock(&inode->i_mutex);
> -
> /* validate length */
> if (len == 0)
> goto out;
>
> - isize = i_size_read(inode);
> - if (!isize)
> - goto out;
> -
> - end_index = (isize - 1) >> huge_page_shift(h);
> for (;;) {
> struct page *page;
> unsigned long nr, ret;
> @@ -263,6 +256,8 @@ static ssize_t hugetlbfs_read(struct fil
>
> /* nr is the maximum number of bytes to copy from this page */
> nr = huge_page_size(h);
> + isize = i_size_read(inode);
> + end_index = isize >> huge_page_shift(h);
Should that be (isize - 1) >> huget_page_shift(h) ?
> if (index >= end_index) {
> if (index > end_index)
> goto out;
> @@ -274,7 +269,7 @@ static ssize_t hugetlbfs_read(struct fil
> nr = nr - offset;
>
> /* Find the page */
> - page = find_get_page(mapping, index);
> + page = find_lock_page(mapping, index);
> if (unlikely(page == NULL)) {
> /*
> * We have a HOLE, zero out the user-buffer for the
> @@ -286,17 +281,30 @@ static ssize_t hugetlbfs_read(struct fil
> else
> ra = 0;
> } else {
> + unlock_page(page);
> +
> + /* Without i_mutex held, check isize again */
> + nr = huge_page_size(h);
> + isize = i_size_read(inode);
> + end_index = isize >> huge_page_shift(h);
same here (isize - 1) ?
> + if (index == end_index) {
> + nr = isize & ~huge_page_mask(h);
Is that correct ? We calculate nr differently in the earlier part of the function
> + if (nr <= offset) {
> + page_cache_release(page);
> + goto out;
> + }
> + }
> + nr -= offset;
> /*
> * We have the page, copy it to user space buffer.
> */
> ra = hugetlbfs_read_actor(page, offset, buf, len, nr);
> ret = ra;
> + page_cache_release(page);
> }
> if (ra < 0) {
> if (retval == 0)
> retval = ra;
> - if (page)
> - page_cache_release(page);
> goto out;
> }
>
> @@ -306,16 +314,12 @@ static ssize_t hugetlbfs_read(struct fil
> index += offset >> huge_page_shift(h);
> offset &= ~huge_page_mask(h);
>
> - if (page)
> - page_cache_release(page);
> -
> /* short read or no more work */
> if ((ret != nr) || (len == 0))
> break;
> }
> out:
> *ppos = ((loff_t)index << huge_page_shift(h)) + offset;
> - mutex_unlock(&inode->i_mutex);
> return retval;
> }
>
I guess we need to closely look at the patch with respect to end-of-file
condition. I will also try to get some testing with the patch.
-aneesh
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@xxxxxxxxx. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@xxxxxxxxx";> email@xxxxxxxxx </a>