Re: [PATCH 03/14] mm: Protect operations adding pages to page cache with invalidate_lock

[Jan Kara <jack@suse.cz>]

On Tue 13-07-21 07:25:05, Christoph Hellwig wrote:
> Still looks good.  That being said the additional conditional locking in
> filemap_fault makes it fall over the readbility cliff for me.  Something
> like this on top of your series would help:

Yeah, that's better. Applied, thanks. 

								Honza

> 
> diff --git a/mm/filemap.c b/mm/filemap.c
> index fd3f94d36c49..0fad08331cf4 100644
> --- a/mm/filemap.c
> +++ b/mm/filemap.c
> @@ -3040,21 +3040,23 @@ vm_fault_t filemap_fault(struct vm_fault *vmf)
>  	 * Do we have something in the page cache already?
>  	 */
>  	page = find_get_page(mapping, offset);
> -	if (likely(page) && !(vmf->flags & FAULT_FLAG_TRIED)) {
> +	if (likely(page)) {
>  		/*
> -		 * We found the page, so try async readahead before
> -		 * waiting for the lock.
> +		 * We found the page, so try async readahead before waiting for
> +		 * the lock.
>  		 */
> -		fpin = do_async_mmap_readahead(vmf, page);
> -	} else if (!page) {
> +		if (!(vmf->flags & FAULT_FLAG_TRIED))
> +			fpin = do_async_mmap_readahead(vmf, page);
> +		if (unlikely(!PageUptodate(page))) {
> +			filemap_invalidate_lock_shared(mapping);
> +			mapping_locked = true;
> +		}
> +	} else {
>  		/* No page in the page cache at all */
>  		count_vm_event(PGMAJFAULT);
>  		count_memcg_event_mm(vmf->vma->vm_mm, PGMAJFAULT);
>  		ret = VM_FAULT_MAJOR;
>  		fpin = do_sync_mmap_readahead(vmf);
> -	}
> -
> -	if (!page) {
>  retry_find:
>  		/*
>  		 * See comment in filemap_create_page() why we need
> @@ -3073,9 +3075,6 @@ vm_fault_t filemap_fault(struct vm_fault *vmf)
>  			filemap_invalidate_unlock_shared(mapping);
>  			return VM_FAULT_OOM;
>  		}
> -	} else if (unlikely(!PageUptodate(page))) {
> -		filemap_invalidate_lock_shared(mapping);
> -		mapping_locked = true;
>  	}
>  
>  	if (!lock_page_maybe_drop_mmap(vmf, page, &fpin))
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR