Re: [PATCH 4/5] mm/rmap: share the i_mmap_rwsem

[Hugh Dickins]

On Thu, 22 May 2014, Davidlohr Bueso wrote:

> Similarly to rmap_walk_anon() and collect_procs_anon(),
> there is opportunity to share the lock in rmap_walk_file()
> and collect_procs_file() for file backed pages.

And lots of other places, no?  I welcome i_mmap_rwsem, but I think
you're approaching it wrongly to separate this off from 2/5, then
follow anon_vma for the places that can be converted to lock_read().

If you go back through 2/5 and study the context of each, I think
you'll find most make no modification to the tree, and can well
use the lock_read() rather than the lock_write().

I could be wrong, but I don't think there are any hidden gotchas.
There certainly are in the anon_vma case (where THP makes special
use of the anon_vma lock), and used to be in the i_mmap_lock case
(when invalidation had to be single-threaded across cond_rescheds),
but I think i_mmap_rwsem should be straightforward.

Sure, it's safe to use the lock_write() variant, but please don't
prefer it to lock_read() without good reason.

Hugh

> 
> Signed-off-by: Davidlohr Bueso <davidlohr@xxxxxx>
> ---
>  include/linux/fs.h  | 10 ++++++++++
>  mm/memory-failure.c |  4 ++--
>  mm/rmap.c           |  4 ++--
>  3 files changed, 14 insertions(+), 4 deletions(-)
> 
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index 60a1d7d..4c2c228 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -467,6 +467,16 @@ static inline void i_mmap_unlock_write(struct address_space *mapping)
>  	up_write(&mapping->i_mmap_rwsem);
>  }
>  
> +static inline void i_mmap_lock_read(struct address_space *mapping)
> +{
> +	down_read(&mapping->i_mmap_rwsem);
> +}
> +
> +static inline void i_mmap_unlock_read(struct address_space *mapping)
> +{
> +	up_read(&mapping->i_mmap_rwsem);
> +}
> +
>  /*
>   * Might pages of this file be mapped into userspace?
>   */
> diff --git a/mm/memory-failure.c b/mm/memory-failure.c
> index 1389a28..acbcd8e 100644
> --- a/mm/memory-failure.c
> +++ b/mm/memory-failure.c
> @@ -434,7 +434,7 @@ static void collect_procs_file(struct page *page, struct list_head *to_kill,
>  	struct task_struct *tsk;
>  	struct address_space *mapping = page->mapping;
>  
> -	i_mmap_lock_write(mapping);
> +	i_mmap_lock_read(mapping);
>  	read_lock(&tasklist_lock);
>  	for_each_process(tsk) {
>  		pgoff_t pgoff = page_pgoff(page);
> @@ -456,7 +456,7 @@ static void collect_procs_file(struct page *page, struct list_head *to_kill,
>  		}
>  	}
>  	read_unlock(&tasklist_lock);
> -	i_mmap_unlock_write(mapping);
> +	i_mmap_unlock_read(mapping);
>  }
>  
>  /*
> diff --git a/mm/rmap.c b/mm/rmap.c
> index 9a56e4f..5841dcb 100644
> --- a/mm/rmap.c
> +++ b/mm/rmap.c
> @@ -1685,7 +1685,7 @@ static int rmap_walk_file(struct page *page, struct rmap_walk_control *rwc)
>  
>  	if (!mapping)
>  		return ret;
> -	i_mmap_lock_write(mapping);
> +	i_mmap_lock_read(mapping);
>  	vma_interval_tree_foreach(vma, &mapping->i_mmap, pgoff, pgoff) {
>  		unsigned long address = vma_address(page, vma);
>  
> @@ -1708,7 +1708,7 @@ static int rmap_walk_file(struct page *page, struct rmap_walk_control *rwc)
>  	ret = rwc->file_nonlinear(page, mapping, rwc->arg);
>  
>  done:
> -	i_mmap_unlock_write(mapping);
> +	i_mmap_unlock_read(mapping);
>  	return ret;
>  }
>  
> -- 
> 1.8.1.4
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/