Re: [PATCH v3 3/5] mm: Drop first_index/last_index in zap_details

[Peter Xu]

On Thu, Sep 09, 2021 at 02:54:37AM +0000, Liam Howlett wrote:
> > @@ -3390,17 +3393,17 @@ void unmap_mapping_page(struct page *page)
> >  void unmap_mapping_pages(struct address_space *mapping, pgoff_t start,
> >  		pgoff_t nr, bool even_cows)
> >  {
> > +	pgoff_t	first_index = start, last_index = start + nr - 1;
> 
> Nit: If you respin, can first_index and last_index be two lines please?

Sure.

> 
> >  	struct zap_details details = { };
> >  
> >  	details.check_mapping = even_cows ? NULL : mapping;
> > -	details.first_index = start;
> > -	details.last_index = start + nr - 1;
> > -	if (details.last_index < details.first_index)
> > -		details.last_index = ULONG_MAX;
> 
> Nit: Maybe throw a comment about this being overflow check, if you
> respin.

It may not be "only" an overflow check, e.g., both unmap_mapping_range() and
unmap_mapping_pages() allows taking the npages to be zero:

For unmap_mapping_range:

 * @holelen: size of prospective hole in bytes.  This will be rounded
 * up to a PAGE_SIZE boundary.  A holelen of zero truncates to the
 * end of the file.

For unmap_mapping_pages:

 * @nr: Number of pages to be unmapped.  0 to unmap to end of file.

So we must set it to ULONG_MAX to make sure nr==0 will work like that.

I won't bother adding a comment, but if to add it I'll probably also mention
about that part on allowing a nr==0 use case, please let me know if you insist.

> 
> > +	if (last_index < first_index)
> > +		last_index = ULONG_MAX;
> >  
> >  	i_mmap_lock_write(mapping);
> >  	if (unlikely(!RB_EMPTY_ROOT(&mapping->i_mmap.rb_root)))
> > -		unmap_mapping_range_tree(&mapping->i_mmap, &details);
> > +		unmap_mapping_range_tree(&mapping->i_mmap, first_index,
> > +					 last_index, &details);
> >  	i_mmap_unlock_write(mapping);
> >  }
> >  
> > -- 
> > 2.31.1
> > 
> 
> Reviewed-by: Liam R. Howlett <Liam.Howlett@xxxxxxxxxx>

Thanks for reviewing.

-- 
Peter Xu