Re: [patch][rfc] mm: new address space calls

[Christoph Hellwig <hch@infradead.org>]

On Wed, Feb 25, 2009 at 11:48:39AM +0100, Nick Piggin wrote:
> This is about the last change to generic code I need for fsblock.
> Comments?
> 
> Introduce new address space operations sync and release, which can be used
> by a filesystem to synchronize and release per-address_space private metadata.
> They generalise sync_mapping_buffers, invalidate_inode_buffers, and
> remove_inode_buffers calls, and get another step closer to divorcing
> buffer heads from core mm/fs code.

>  void invalidate_inode_buffers(struct inode *inode)
>  {
> -	if (inode_has_buffers(inode)) {
> -		struct address_space *mapping = &inode->i_data;
> +	struct address_space *mapping = &inode->i_data;
> +
> +	if (mapping_has_private(mapping)) {
>  		struct list_head *list = &mapping->private_list;
>  		struct address_space *buffer_mapping = mapping->assoc_mapping;

I'ts not really helping much here as we still directly poke into the
buffer_head list.

> --- linux-2.6.orig/fs/fs-writeback.c
> +++ linux-2.6/fs/fs-writeback.c
> @@ -782,9 +782,15 @@ int generic_osync_inode(struct inode *in
>  	if (what & OSYNC_DATA)
>  		err = filemap_fdatawrite(mapping);
>  	if (what & (OSYNC_METADATA|OSYNC_DATA)) {
> -		err2 = sync_mapping_buffers(mapping);
> -		if (!err)
> -			err = err2;
> +		if (!mapping->a_ops->sync) {
> +			err2 = sync_mapping_buffers(mapping);
> +			if (!err)
> +				err = err2;
> +		} else {
> +			err2 = mapping->a_ops->sync(mapping);
> +			if (!err)
> +				err = err2;
> +		}
>  	}
>  	if (what & OSYNC_DATA) {
>  		err2 = filemap_fdatawait(mapping);

I'd really prefer not having the default fallbacks, these kinds
of implicit fallbacks make the code really hard to maintain over
long term.

I also wonder if moving the filemap_fdatawrite/filemap_fdatawait
into the method would help.  In fact it's surprisingly similar
to ->fsync in many ways, that I wonder if these should be one
operation.

>   */
>  void clear_inode(struct inode *inode)
>  {
> +	struct address_space *mapping = &inode->i_data;
> +
>  	might_sleep();
> -	invalidate_inode_buffers(inode);
> +	if (!mapping->a_ops->release)
> +		invalidate_inode_buffers(inode);

That's a weird one.  The implict default shouldn't be in a different
place from the method.