Re: [rfc][patch 3/4] fs: new truncate sequence

[Nick Piggin]

On Tue, Jul 07, 2009 at 12:30:42PM -0400, Christoph Hellwig wrote:
> On Tue, Jul 07, 2009 at 05:48:09PM +0200, Nick Piggin wrote:
> > OK, so what do you suggest? If the filesystem defines
> > ->setsize then do not pass ATTR_SIZE changes into setattr?
> > But then do you also not pass in ATTR_TIME cchanges to setattr
> > iff they  are together with ATTR_SIZE change? It sees also like
> > quite a difficult calling convention.
> 
> Ok, I played around with these ideas and your patches a bit.  I think
> we're actually best of to return to one of the early ideas and just
> get rid of ->truncate without any replacement, e.g. let ->setattr
> handle all of it.

Yes I do agree. ->setsize inside inode_setattr would have been
OK as wel I think, but this is probably even cleaner even though
it might be a bit more work.

 
> Below is a patch ontop of you four patches that implements exactly that
> and it looks surprisingly nice.  The only gotcha I can see is that we
> need to audit for existing filesystems not implementing ->truncate
> getting a behaviour change due to the checks to decide if we want
> to call vmtruncate.  But most likely any existing filesystems without
> ->truncate using the buffer.c helper or direct I/O is buggy anyway.

Thanks for the patch, I think I will fold it in to the series. I
think we probably do need to call simple_setsize in inode_setattr
though (unless you propose to eventually convert every filesystem
to define a .setattr). This would also require eg. your ext2
conversion to strip ATTR_SIZE before passing through to inode_setattr.

We could just add some temporary field for example in the i_op
structure to test for and remove it when everybody is converted,
which woud guarantee back compatibility.


> Note that it doesn't touch i_alloc_mutex locking for now - if we go
> down this route I would do the lock shift in one patch at the end of
> the series.

Yeah fine by me (or do it in a new series).

 
>  int ext2_setattr(struct dentry *dentry, struct iattr *iattr)
>  {
>  	struct inode *inode = dentry->d_inode;
>  	int error;
>  
> +	if (iattr->ia_valid & ATTR_SIZE) {
> +		error = ext2_setsize(inode, iattr->ia_size);
> +		if (error)
> +			return error;
> +	}
> +
>  	error = inode_change_ok(inode, iattr);
>  	if (error)
>  		return error;

Probably want to call inode_change_ok first here.

--
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/