Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1260029 > unrolled thread

Re: [PATCH] mm/hugetlbfs Fix bugs in fallocate hole punch of areas with holes

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH] mm/hugetlbfs Fix bugs in fallocate hole punch of areas  with holes Mike Kravetz <mike.kravetz@oracle.com> - 2015-10-31 19:00 +0100
    Re: [PATCH] mm/hugetlbfs Fix bugs in fallocate hole punch of areas with holes "Hillf Danton" <hillf.zj@alibaba-inc.com> - 2015-11-02 04:00 +0100
      Re: [PATCH] mm/hugetlbfs Fix bugs in fallocate hole punch of areas  with holes Mike Kravetz <mike.kravetz@oracle.com> - 2015-11-02 18:40 +0100
        Re: [PATCH] mm/hugetlbfs Fix bugs in fallocate hole punch of areas  with holes Andrew Morton <akpm@linux-foundation.org> - 2015-11-02 22:50 +0100

#1260029 — Re: [PATCH] mm/hugetlbfs Fix bugs in fallocate hole punch of areas with holes

On 10/30/2015 10:07 PM, Hillf Danton wrote:
>>
>> Hugh Dickins pointed out problems with the new hugetlbfs fallocate
>> hole punch code.  These problems are in the routine remove_inode_hugepages
>> and mostly occur in the case where there are holes in the range of
>> pages to be removed.  These holes could be the result of a previous hole
>> punch or simply sparse allocation.
>>
>> remove_inode_hugepages handles both hole punch and truncate operations.
>> Page index handling was fixed/cleaned up so that holes are properly
>> handled.  In addition, code was changed to ensure multiple passes of the
>> address range only happens in the truncate case.  More comments were added
>> to explain the different actions in each case.  A cond_resched() was added
>> after removing up to PAGEVEC_SIZE pages.
>>
>> Some totally unnecessary code in hugetlbfs_fallocate() that remained from
>> early development was also removed.
>>
>> Signed-off-by: Mike Kravetz <mike.kravetz@oracle.com>
>> ---
>>  fs/hugetlbfs/inode.c | 44 +++++++++++++++++++++++++++++---------------
>>  1 file changed, 29 insertions(+), 15 deletions(-)
>>
>> diff --git a/fs/hugetlbfs/inode.c b/fs/hugetlbfs/inode.c
>> index 316adb9..30cf534 100644
>> --- a/fs/hugetlbfs/inode.c
>> +++ b/fs/hugetlbfs/inode.c
>> @@ -368,10 +368,25 @@ static void remove_inode_hugepages(struct inode *inode, loff_t lstart,
>>  			lookup_nr = end - next;
>>
>>  		/*
>> -		 * This pagevec_lookup() may return pages past 'end',
>> -		 * so we must check for page->index > end.
>> +		 * When no more pages are found, take different action for
>> +		 * hole punch and truncate.
>> +		 *
>> +		 * For hole punch, this indicates we have removed each page
>> +		 * within the range and are done.  Note that pages may have
>> +		 * been faulted in after being removed in the hole punch case.
>> +		 * This is OK as long as each page in the range was removed
>> +		 * once.
>> +		 *
>> +		 * For truncate, we need to make sure all pages within the
>> +		 * range are removed when exiting this routine.  We could
>> +		 * have raced with a fault that brought in a page after it
>> +		 * was first removed.  Check the range again until no pages
>> +		 * are found.
>>  		 */
>>  		if (!pagevec_lookup(&pvec, mapping, next, lookup_nr)) {
>> +			if (!truncate_op)
>> +				break;
>> +
>>  			if (next == start)
>>  				break;
>>  			next = start;
>> @@ -382,19 +397,23 @@ static void remove_inode_hugepages(struct inode *inode, loff_t lstart,
>>  			struct page *page = pvec.pages[i];
>>  			u32 hash;
>>
>> +			/*
>> +			 * The page (index) could be beyond end.  This is
>> +			 * only possible in the punch hole case as end is
>> +			 * LLONG_MAX for truncate.
>> +			 */
>> +			if (page->index >= end) {
>> +				next = end;	/* we are done */
>> +				break;
>> +			}
>> +			next = page->index;
>> +
>>  			hash = hugetlb_fault_mutex_hash(h, current->mm,
>>  							&pseudo_vma,
>>  							mapping, next, 0);
>>  			mutex_lock(&hugetlb_fault_mutex_table[hash]);
>>
>>  			lock_page(page);
>> -			if (page->index >= end) {
>> -				unlock_page(page);
>> -				mutex_unlock(&hugetlb_fault_mutex_table[hash]);
>> -				next = end;	/* we are done */
>> -				break;
>> -			}
>> -
>>  			/*
>>  			 * If page is mapped, it was faulted in after being
>>  			 * unmapped.  Do nothing in this race case.  In the
>> @@ -423,15 +442,13 @@ static void remove_inode_hugepages(struct inode *inode, loff_t lstart,
>>  				}
>>  			}
>>
>> -			if (page->index > next)
>> -				next = page->index;
>> -
>>  			++next;
>>  			unlock_page(page);
>>
>>  			mutex_unlock(&hugetlb_fault_mutex_table[hash]);
>>  		}
>>  		huge_pagevec_release(&pvec);
>> +		cond_resched();
>>  	}
>>
>>  	if (truncate_op)
>> @@ -647,9 +664,6 @@ static long hugetlbfs_fallocate(struct file *file, int mode, loff_t offset,
> 
> This hunk is already in the next tree, see below please.
> 

Ah, the whole series to add shmem like code to handle hole punch/fault
races is in the next tree.  It has been determined that most of this
series is not necessary.  For the next tree, ideally the following
should happen:
- revert the series
	0830d5afd4ab69d01cf5ceba9b9f2796564c4eb6
	4e0a78fea078af972276c2d3aeaceb2bac80e033
	251c8a023a0c639725e014a612e8c05a631ce839
	03bcef375766af4db12ec783241ac39f8bf5e2b1
- Add this patch (if Ack'ed/reviewed) to fix remove_inode_hugepages
- Add a new patch for the handle hole punch/fault race.  It modifies
  same code as this patch, so I have not sent out until this is Ack'ed.

I will admit that I do not fully understand how maintainers manage their
trees and share patches.  If someone can make suggestions on how to handle
this situation (create patches against what tree? send patches to who?),
I will be happy to make it happen.

-- 
Mike Kravetz

>>  	if (!(mode & FALLOC_FL_KEEP_SIZE) && offset + len > inode->i_size)
>>  		i_size_write(inode, offset + len);
>>  	inode->i_ctime = CURRENT_TIME;
>> -	spin_lock(&inode->i_lock);
>> -	inode->i_private = NULL;
>> -	spin_unlock(&inode->i_lock);
>>  out:
>>  	mutex_unlock(&inode->i_mutex);
>>  	return error;
>> --
>> 2.4.3
>>
> In the next tree,
> 	4e0a78fea078af972276c2d3aeaceb2bac80e033
> 	mm/hugetlb: setup hugetlb_falloc during fallocate hole punch
> 
> @@ -647,9 +676,6 @@ static long hugetlbfs_fallocate(struct file *file, int mode, loff_t offset,
>  	if (!(mode & FALLOC_FL_KEEP_SIZE) && offset + len > inode->i_size)
>  		i_size_write(inode, offset + len);
>  	inode->i_ctime = CURRENT_TIME;
> -	spin_lock(&inode->i_lock);
> -	inode->i_private = NULL;
> -	spin_unlock(&inode->i_lock);
>  out:
>  	mutex_unlock(&inode->i_mutex);
>  	return error;
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1260309 — Re: [PATCH] mm/hugetlbfs Fix bugs in fallocate hole punch of areas with holes

Andrew, please correct me if I miss/mess anything.
 
> > This hunk is already in the next tree, see below please.
> >
> 
> Ah, the whole series to add shmem like code to handle hole punch/fault
> races is in the next tree.  It has been determined that most of this
> series is not necessary.  For the next tree, ideally the following
> should happen:
> - revert the series
> 	0830d5afd4ab69d01cf5ceba9b9f2796564c4eb6
> 	4e0a78fea078af972276c2d3aeaceb2bac80e033
> 	251c8a023a0c639725e014a612e8c05a631ce839
> 	03bcef375766af4db12ec783241ac39f8bf5e2b1
> - Add this patch (if Ack'ed/reviewed) to fix remove_inode_hugepages
> - Add a new patch for the handle hole punch/fault race.  It modifies
>   same code as this patch, so I have not sent out until this is Ack'ed.
> 
> I will admit that I do not fully understand how maintainers manage their
> trees and share patches.  If someone can make suggestions on how to handle
> this situation (create patches against what tree? send patches to who?),
> I will be happy to make it happen.
> 
The rule is to prepare patches against the next tree and deliver patches to
linux-mm with AKPM, linux-kernel cced. The authors and maintainers of the
current code your patches change should also be cced.
And those guys you want to get ack and comments.

In this case, you should first ask Andrew to withdraw the 4 commits.
Then send your new patches, one after another, one problem a patch.

Best Wishes
Hillf

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1260826

On 11/01/2015 06:50 PM, Hillf Danton wrote:
> Andrew, please correct me if I miss/mess anything.
>  
>>> This hunk is already in the next tree, see below please.
>>>
>>
>> Ah, the whole series to add shmem like code to handle hole punch/fault
>> races is in the next tree.  It has been determined that most of this
>> series is not necessary.  For the next tree, ideally the following
>> should happen:
>> - revert the series
>> 	0830d5afd4ab69d01cf5ceba9b9f2796564c4eb6
>> 	4e0a78fea078af972276c2d3aeaceb2bac80e033
>> 	251c8a023a0c639725e014a612e8c05a631ce839
>> 	03bcef375766af4db12ec783241ac39f8bf5e2b1
>> - Add this patch (if Ack'ed/reviewed) to fix remove_inode_hugepages
>> - Add a new patch for the handle hole punch/fault race.  It modifies
>>   same code as this patch, so I have not sent out until this is Ack'ed.
>>
>> I will admit that I do not fully understand how maintainers manage their
>> trees and share patches.  If someone can make suggestions on how to handle
>> this situation (create patches against what tree? send patches to who?),
>> I will be happy to make it happen.
>>
> The rule is to prepare patches against the next tree and deliver patches to
> linux-mm with AKPM, linux-kernel cced. The authors and maintainers of the
> current code your patches change should also be cced.
> And those guys you want to get ack and comments.
> 
> In this case, you should first ask Andrew to withdraw the 4 commits.
> Then send your new patches, one after another, one problem a patch.
> 
> Best Wishes
> Hillf

Andrew,

As mentioned above, it has been determined that most of the series titled
"[PATCH v2 0/4] hugetlbfs fallocate hole punch race with page faults" is
unnecessary.  Ideally, we want to remove this entire series from mmotm and
linux-next.  It  will be replaced with a simpler patch.

However, before that happens I would like to address bugs in the current
code as pointed out by Hugh Dickins.  These are addresses in the patch
which started this thread:
"[PATCH] mm/hugetlbfs Fix bugs in fallocate hole punch of areas with holes"

If there is a way to do this that makes your work easier let me know.

It would be good if someone could take a look at this patch as these bugs
also exist in 4.3.


-- 
Mike Kravetz
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1261005

On Mon, 2 Nov 2015 09:39:14 -0800 Mike Kravetz <mike.kravetz@oracle.com> wrote:

> On 11/01/2015 06:50 PM, Hillf Danton wrote:
> > Andrew, please correct me if I miss/mess anything.
> >  
> >>> This hunk is already in the next tree, see below please.
> >>>
> >>
> >> Ah, the whole series to add shmem like code to handle hole punch/fault
> >> races is in the next tree.  It has been determined that most of this
> >> series is not necessary.  For the next tree, ideally the following
> >> should happen:
> >> - revert the series
> >> 	0830d5afd4ab69d01cf5ceba9b9f2796564c4eb6
> >> 	4e0a78fea078af972276c2d3aeaceb2bac80e033
> >> 	251c8a023a0c639725e014a612e8c05a631ce839
> >> 	03bcef375766af4db12ec783241ac39f8bf5e2b1
> >> - Add this patch (if Ack'ed/reviewed) to fix remove_inode_hugepages
> >> - Add a new patch for the handle hole punch/fault race.  It modifies
> >>   same code as this patch, so I have not sent out until this is Ack'ed.
> >>
> >> I will admit that I do not fully understand how maintainers manage their
> >> trees and share patches.  If someone can make suggestions on how to handle
> >> this situation (create patches against what tree? send patches to who?),
> >> I will be happy to make it happen.
> >>
> > The rule is to prepare patches against the next tree and deliver patches to
> > linux-mm with AKPM, linux-kernel cced. The authors and maintainers of the
> > current code your patches change should also be cced.
> > And those guys you want to get ack and comments.
> > 
> > In this case, you should first ask Andrew to withdraw the 4 commits.
> > Then send your new patches, one after another, one problem a patch.
> > 
> > Best Wishes
> > Hillf
> 
> Andrew,
> 
> As mentioned above, it has been determined that most of the series titled
> "[PATCH v2 0/4] hugetlbfs fallocate hole punch race with page faults" is
> unnecessary.  Ideally, we want to remove this entire series from mmotm and
> linux-next.  It  will be replaced with a simpler patch.

I dropped them all.

> However, before that happens I would like to address bugs in the current
> code as pointed out by Hugh Dickins.  These are addresses in the patch
> which started this thread:
> "[PATCH] mm/hugetlbfs Fix bugs in fallocate hole punch of areas with holes"

And merged that.  With a note reminding myself to get a Hugh ack ;)


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web