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
