Re: [patch 1/5] fs: truncate introduce new sequence

[Jan Kara <jack@suse.cz>]

  Hi,

On Tue 08-12-09 09:38:32, Nick Piggin wrote:
> Changes: avoid .truncate_kludge_to_be_killed hack by implementing additional
> fs helper functions that do not call vmtruncate.
> 
> So far have converted ext2 and fat, tested with fsx-linux and the recent
> writev truncate bug tester. The reset of the patches that were already
> made are pretty simple to convert but I will likely prefer to send those
> via their maintainers after this initial set is agreed and merged.
> 
> The new helpers, *_newtrunc, are a bit ugly, but we can do a rename back
> to the old names after all support for old sequence is removed.
  Yeah. I'm not sure what Al dislikes about the original temporary hack.
Maybe he's afraid that the hack will last longer than we'd like... What I
dislike about this approach is that we'll have to change every filesystem
in several places also in the second patch. Anyway I'd like to get this
change merged and I don't care too much about details of the intermediate
period...

> --
> Introduce a new truncate calling sequence into fs/mm subsystems. Rather than
> setattr > vmtruncate > truncate, have filesystems call their truncate sequence
> from ->setattr if filesystem specific operations are required. vmtruncate is
> deprecated, and truncate_pagecache and inode_newsize_ok helpers introduced
> previously should be used.
> 
> simple_setattr is introduced for simple in-ram filesystems to implement
> the new truncate sequence. Eventually all filesystems should be converted
> to implement a setattr, and the default code in notify_change should go
> away.
> 
> simple_setsize is also introduced to perform just the ATTR_SIZE portion
> of simple_setattr (ie. changing i_size and trimming pagecache).
> 
> To implement the new truncate sequence:
> - filesystem specific manipulations (eg freeing blocks) must be done in
>   the setattr method rather than ->truncate.
> - vmtruncate can not be used by core code to trim blocks past i_size in
>   the event of write failure after allocation, so this must be performed
>   in the fs code.
> - convert usage of helpers block_write_begin, nobh_write_begin,
>   cont_write_begin, and *blockdev_direct_IO* to use _newtrunc postfixed
>   variants. These avoid calling vmtruncate to trim blocks (see previous).
> - inode_setattr should not be used. generic_setattr is a new function
>   to be used to copy simple attributes into the generic inode.
> - make use of the better opportunity to handle errors with the new sequence.
> 
> Big problem with the previous calling sequence: the filesystem is not called
> until i_size has already changed.  This means it is not allowed to fail the
> call, and also it does not know what the previous i_size was. Also, generic
> code calling vmtruncate to truncate allocated blocks in case of error had
> no good way to return a meaningful error (or, for example, atomically handle
> block deallocation).
> 
> Cc: Christoph Hellwig <hch@lst.de>
> Signed-off-by: Nick Piggin <npiggin@suse.de>
  The patch looks fine except for a typo below.
Acked-by: Jan Kara <jack@suse.cz>

									Honza
> ---
>  Documentation/filesystems/vfs.txt |    7 +-
>  fs/attr.c                         |   50 ++++++++++++---
>  fs/buffer.c                       |  123 ++++++++++++++++++++++++++++++--------
>  fs/direct-io.c                    |   85 ++++++++++++++++----------
>  fs/libfs.c                        |   83 +++++++++++++++++++++++++
>  include/linux/buffer_head.h       |    9 ++
>  include/linux/fs.h                |   36 ++++++++++-
>  mm/truncate.c                     |   10 +--
>  8 files changed, 328 insertions(+), 75 deletions(-)
> 
> Index: linux-2.6/Documentation/filesystems/vfs.txt
> ===================================================================
> --- linux-2.6.orig/Documentation/filesystems/vfs.txt
> +++ linux-2.6/Documentation/filesystems/vfs.txt
> @@ -401,11 +401,16 @@ otherwise noted.
>    	started might not be in the page cache at the end of the
>    	walk).
>  
> -  truncate: called by the VFS to change the size of a file.  The
> +  truncate: Deprecated. This will not be called if ->setsize is defined.
> +	Called by the VFS to change the size of a file.  The
>   	i_size field of the inode is set to the desired size by the
>   	VFS before this method is called.  This method is called by
>   	the truncate(2) system call and related functionality.
>  
> +	Note: ->truncate and vmtruncate are deprecated. Do not add new
> +	instances/calls of these. Filesystems shoud be converted to do their
                                              ^^^^^ should
-- 
Jan Kara <jack@suse.cz>
SUSE Labs, CR