Re: [RFC] exofs: New truncate sequence

[Nick Piggin <npiggin@suse.de>]

On Mon, May 31, 2010 at 03:30:02PM +0300, Boaz Harrosh wrote:
> 
> These changes are crafted based on the similar
> conversion done to ext2 by Nick Piggin.
> 
> Please check me out I bound to have miss-looked
> something.
> 
> - Remove the deprecated ->truncate vector, call a new
>   exofs_setsize for on-disk size update, from exofs_setattr
> - Call truncate_pagecache on the unused pages if
>   write_begin/end fails.
> - Cleanup exofs_delete_inode that did stupid inode
>   writes and updates on an inode that will be
>   removed.
> - And finally get rid of exofs_get_block. We never
>   had any blocks it was all for calling nobh_truncate_page.
>   nobh_truncate_page is not actually needed in exofs since
>   the last page is complete and gone just like all the other
>   pages. There is no partial blocks in exofs.
>   [OK do I might need a partial read here upto i_size ???]
> 
> I've tested with this patch, and there are no apparent
> failures, so far.
> 
> These patches are based on Al's vfs for-next branch, which
> contain Nick's last patches. But they do not contain,
> (and will lightly conflict with) latest Christoph's
> patches. Christoph do you have a tree I can rebase on? or
> maybe you want to take this patch into your patchset?
> 
> CC: Christoph Hellwig <hch@lst.de>
> CC: Nick Piggin <npiggin@suse.de>
> Signed-off-by: Boaz Harrosh <bharrosh@panasas.com>
> ---
>  fs/exofs/exofs.h |    1 -
>  fs/exofs/file.c  |    1 -
>  fs/exofs/inode.c |  115 +++++++++++++++++++++++++++---------------------------

Can you rip out all the rest of the buffer_head stuff too?


>  3 files changed, 58 insertions(+), 59 deletions(-)
> 
> diff --git a/fs/exofs/exofs.h b/fs/exofs/exofs.h
> index 22721b2..0706ce9 100644
> --- a/fs/exofs/exofs.h
> +++ b/fs/exofs/exofs.h
> @@ -256,7 +256,6 @@ static inline int exofs_oi_read(struct exofs_i_info *oi,
>  }
>  
>  /* inode.c               */
> -void exofs_truncate(struct inode *inode);
>  int exofs_setattr(struct dentry *, struct iattr *);
>  int exofs_write_begin(struct file *file, struct address_space *mapping,
>  		loff_t pos, unsigned len, unsigned flags,
> diff --git a/fs/exofs/file.c b/fs/exofs/file.c
> index fef6899..f9bfe2b 100644
> --- a/fs/exofs/file.c
> +++ b/fs/exofs/file.c
> @@ -86,6 +86,5 @@ const struct file_operations exofs_file_operations = {
>  };
>  
>  const struct inode_operations exofs_file_inode_operations = {
> -	.truncate	= exofs_truncate,
>  	.setattr	= exofs_setattr,
>  };
> diff --git a/fs/exofs/inode.c b/fs/exofs/inode.c
> index 4bb6ef8..1c2666c 100644
> --- a/fs/exofs/inode.c
> +++ b/fs/exofs/inode.c
> @@ -710,7 +710,7 @@ int exofs_write_begin(struct file *file, struct address_space *mapping,
>  					 fsdata);
>  		if (ret) {
>  			EXOFS_DBGMSG("simple_write_begin faild\n");
> -			return ret;
> +			goto out;
>  		}
>  
>  		page = *pagep;
> @@ -725,7 +725,17 @@ int exofs_write_begin(struct file *file, struct address_space *mapping,
>  			EXOFS_DBGMSG("__readpage_filler faild\n");
>  		}
>  	}
> +out:
> +	if (unlikely(ret)) {
> +		struct inode *inode = mapping->host;
> +		loff_t to = pos + len;
>  
> +		if (to > inode->i_size)
> +			truncate_pagecache(inode, to, inode->i_size);
> +
> +		mark_inode_dirty(inode); /* write the new size */

Is this required? I don't think the i_size should be changed here?

> +		return ret;
> +	}
>  	return ret;
>  }
>  
> @@ -750,6 +760,10 @@ static int exofs_write_end(struct file *file, struct address_space *mapping,
>  	int ret;
>  
>  	ret = simple_write_end(file, mapping,pos, len, copied, page, fsdata);
> +	if (unlikely(ret && pos + len > inode->i_size))
> +		truncate_pagecache(inode, pos + len, inode->i_size);
> +

So there is no need to do any oi_truncate? Even if _readpage in
write_begin has set up some blocks?


> +	/* TODO: once simple_write_end marks inode dirty remove */
>  	if (i_size != inode->i_size)
>  		mark_inode_dirty(inode);
>  	return ret;

Hmm, I suppose simple_write_end probably should mark the inode dirty?


> @@ -1335,28 +1339,25 @@ void exofs_delete_inode(struct inode *inode)
>  
>  	truncate_inode_pages(&inode->i_data, 0);
>  
> +	/* TODO: should do better here */
>  	if (is_bad_inode(inode))
>  		goto no_delete;
>  
> -	mark_inode_dirty(inode);
> -	exofs_update_inode(inode, inode_needs_sync(inode));
> -
>  	inode->i_size = 0;
> -	if (inode->i_blocks)
> -		exofs_truncate(inode);

This guy has gone missing -- I assume exofs_sbi_remove is a more
efficient way to do this anyway?


> -
>  	clear_inode(inode);
>  
> -	ret = exofs_get_io_state(&sbi->layout, &ios);
> -	if (unlikely(ret)) {
> -		EXOFS_ERR("%s: exofs_get_io_state failed\n", __func__);
> -		return;
> -	}
> -
>  	/* if we are deleting an obj that hasn't been created yet, wait */
>  	if (!obj_created(oi)) {
>  		BUG_ON(!obj_2bcreated(oi));
>  		wait_event(oi->i_wq, obj_created(oi));
> +		/* ignore the error attempt a remove anyway */
> +	}
> +
> +	/* Now Remove the OSD objects */
> +	ret = exofs_get_io_state(&sbi->layout, &ios);
> +	if (unlikely(ret)) {
> +		EXOFS_ERR("%s: exofs_get_io_state failed\n", __func__);
> +		return;
>  	}
>  
>  	ios->obj.id = exofs_oi_objno(oi);
> -- 
> 1.6.6.1