Re: [PATCH 01/15 v2] mm: add invalidatepage_range address space operation

[Andrew Morton <akpm@linux-foundation.org>]

On Fri, 31 Aug 2012 18:21:37 -0400
Lukas Czerner <lczerner@redhat.com> wrote:

> Currently there is no way to truncate partial page where the end
> truncate point is not at the end of the page. This is because it was not
> needed and the functionality was enough for file system truncate
> operation to work properly. However more file systems now support punch
> hole feature and it can benefit from mm supporting truncating page just
> up to the certain point.
> 
> Specifically, with this functionality truncate_inode_pages_range() can
> be changed so it supports truncating partial page at the end of the
> range (currently it will BUG_ON() if 'end' is not at the end of the
> page).
> 
> This commit add new address space operation invalidatepage_range which
> allows specifying length of bytes to invalidate, rather than assuming
> truncate to the end of the page. It also introduce
> block_invalidatepage_range() and do_invalidatepage)range() functions for
> exactly the same reason.
> 
> The caller does not have to implement both aops (invalidatepage and
> invalidatepage_range) and the latter is preferred. The old method will be
> used only if invalidatepage_range is not implemented by the caller.
> 
> ...
>
> +/**
> + * do_invalidatepage_range - invalidate range of the page
> + *
> + * @page: the page which is affected
> + * @offset: start of the range to invalidate
> + * @length: length of the range to invalidate
> +  */
> +void do_invalidatepage_range(struct page *page, unsigned int offset,
> +			     unsigned int length)
> +{
> +	void (*invalidatepage_range)(struct page *, unsigned int,
> +				     unsigned int);
>  	void (*invalidatepage)(struct page *, unsigned long);
> +
> +	/*
> +	 * Try invalidatepage_range first
> +	 */
> +	invalidatepage_range = page->mapping->a_ops->invalidatepage_range;
> +	if (invalidatepage_range) {
> +		(*invalidatepage_range)(page, offset, length);
> +		return;
> +	}
> +
> +	/*
> +	 * When only invalidatepage is registered length + offset must be
> +	 * PAGE_CACHE_SIZE
> +	 */
>  	invalidatepage = page->mapping->a_ops->invalidatepage;
> +	if (invalidatepage) {
> +		BUG_ON(length + offset != PAGE_CACHE_SIZE);
> +		(*invalidatepage)(page, offset);
> +	}
>  #ifdef CONFIG_BLOCK
> -	if (!invalidatepage)
> -		invalidatepage = block_invalidatepage;
> +	if (!invalidatepage_range && !invalidatepage)
> +		block_invalidatepage_range(page, offset, length);
>  #endif
> -	if (invalidatepage)
> -		(*invalidatepage)(page, offset);
>  }

This interface is ...  strange.  If the caller requests a
non-page-aligned invalidateion against an fs which doesn't implement
->invalidatepage_range then the kernel goes BUG.  So the caller must
know beforehand that the underlying fs _does_ implement
->invalidatepage_range.

For practical purposes, this implies that invalidation of a
non-page-aligned region will only be performed by fs code, because the
fs implicitly knows that it implements ->invalidatepage_range.

However this function isn't exported to modules, so scratch that.

So how is calling code supposed to determine whether it can actually
_use_ this interface?



Also...  one would obviously like to see the old ->invalidatepage() get
removed entirely.  But about 20 filesystems implement
->invalidatepage() and implementation of ->invalidatepage_range() is
non-trivial and actually unnecessary.

So I dunno.  Perhaps we should keep ->invalidatepage() and
->invalidatepage_range() completely separate.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>