[patch 4/5] ext2: convert to use the new truncate convention

[patch 4/5] ext2: convert to use the new truncate convention

[Nick Piggin]

I also have commented a possible bug in existing ext2 code, marked with XXX.

Cc: linux-ext4@vger.kernel.org
Cc: Christoph Hellwig <hch@lst.de>
Signed-off-by: Nick Piggin <npiggin@suse.de>
---
 fs/ext2/ext2.h  |    1 
 fs/ext2/file.c  |    1 
 fs/ext2/inode.c |  153 +++++++++++++++++++++++++++++++++++++++++++-------------
 3 files changed, 119 insertions(+), 36 deletions(-)

Index: linux-2.6/fs/ext2/ext2.h
===================================================================
--- linux-2.6.orig/fs/ext2/ext2.h
+++ linux-2.6/fs/ext2/ext2.h
@@ -122,7 +122,6 @@ extern int ext2_write_inode (struct inod
 extern void ext2_delete_inode (struct inode *);
 extern int ext2_sync_inode (struct inode *);
 extern int ext2_get_block(struct inode *, sector_t, struct buffer_head *, int);
-extern void ext2_truncate (struct inode *);
 extern int ext2_setattr (struct dentry *, struct iattr *);
 extern void ext2_set_inode_flags(struct inode *inode);
 extern void ext2_get_inode_flags(struct ext2_inode_info *);
Index: linux-2.6/fs/ext2/file.c
===================================================================
--- linux-2.6.orig/fs/ext2/file.c
+++ linux-2.6/fs/ext2/file.c
@@ -77,7 +77,6 @@ const struct file_operations ext2_xip_fi
 #endif
 
 const struct inode_operations ext2_file_inode_operations = {
-	.truncate	= ext2_truncate,
 #ifdef CONFIG_EXT2_FS_XATTR
 	.setxattr	= generic_setxattr,
 	.getxattr	= generic_getxattr,
Index: linux-2.6/fs/ext2/inode.c
===================================================================
--- linux-2.6.orig/fs/ext2/inode.c
+++ linux-2.6/fs/ext2/inode.c
@@ -53,6 +53,18 @@ static inline int ext2_inode_is_fast_sym
 		inode->i_blocks - ea_blocks == 0);
 }
 
+static void ext2_truncate_blocks(struct inode *inode, loff_t offset);
+
+static void ext2_write_failed(struct address_space *mapping, loff_t to)
+{
+	struct inode *inode = mapping->host;
+
+	if (to > inode->i_size) {
+		truncate_pagecache(inode, to, inode->i_size);
+		ext2_truncate_blocks(inode, inode->i_size);
+	}
+}
+
 /*
  * Called at the last iput() if i_nlink is zero.
  */
@@ -68,7 +80,7 @@ void ext2_delete_inode (struct inode * i
 
 	inode->i_size = 0;
 	if (inode->i_blocks)
-		ext2_truncate (inode);
+		ext2_truncate_blocks(inode, 0);
 	ext2_free_inode (inode);
 
 	return;
@@ -752,8 +764,8 @@ int __ext2_write_begin(struct file *file
 		loff_t pos, unsigned len, unsigned flags,
 		struct page **pagep, void **fsdata)
 {
-	return block_write_begin(file, mapping, pos, len, flags, pagep, fsdata,
-							ext2_get_block);
+	return block_write_begin_newtrunc(file, mapping, pos, len, flags,
+					pagep, fsdata, ext2_get_block);
 }
 
 static int
@@ -761,8 +773,25 @@ ext2_write_begin(struct file *file, stru
 		loff_t pos, unsigned len, unsigned flags,
 		struct page **pagep, void **fsdata)
 {
+	int ret;
+
 	*pagep = NULL;
-	return __ext2_write_begin(file, mapping, pos, len, flags, pagep,fsdata);
+	ret = __ext2_write_begin(file, mapping, pos, len, flags, pagep, fsdata);
+	if (ret < 0)
+		ext2_write_failed(mapping, pos + len);
+	return ret;
+}
+
+static int ext2_write_end(struct file *file, struct address_space *mapping,
+			loff_t pos, unsigned len, unsigned copied,
+			struct page *page, void *fsdata)
+{
+	int ret;
+
+	ret = generic_write_end(file, mapping, pos, len, copied, page, fsdata);
+	if (ret < len)
+		ext2_write_failed(mapping, pos + len);
+	return ret;
 }
 
 static int
@@ -770,13 +799,18 @@ ext2_nobh_write_begin(struct file *file,
 		loff_t pos, unsigned len, unsigned flags,
 		struct page **pagep, void **fsdata)
 {
+	int ret;
+
 	/*
 	 * Dir-in-pagecache still uses ext2_write_begin. Would have to rework
 	 * directory handling code to pass around offsets rather than struct
 	 * pages in order to make this work easily.
 	 */
-	return nobh_write_begin(file, mapping, pos, len, flags, pagep, fsdata,
-							ext2_get_block);
+	ret = nobh_write_begin_newtrunc(file, mapping, pos, len, flags, pagep,
+						fsdata, ext2_get_block);
+	if (ret < 0)
+		ext2_write_failed(mapping, pos + len);
+	return ret;
 }
 
 static int ext2_nobh_writepage(struct page *page,
@@ -795,10 +829,15 @@ ext2_direct_IO(int rw, struct kiocb *ioc
 			loff_t offset, unsigned long nr_segs)
 {
 	struct file *file = iocb->ki_filp;
-	struct inode *inode = file->f_mapping->host;
-
-	return blockdev_direct_IO(rw, iocb, inode, inode->i_sb->s_bdev, iov,
-				offset, nr_segs, ext2_get_block, NULL);
+	struct address_space *mapping = file->f_mapping;
+	struct inode *inode = mapping->host;
+	ssize_t ret;
+
+	ret = blockdev_direct_IO_newtrunc(rw, iocb, inode, inode->i_sb->s_bdev,
+				iov, offset, nr_segs, ext2_get_block, NULL);
+	if (ret < 0 && (rw & WRITE))
+		ext2_write_failed(mapping, offset + iov_length(iov, nr_segs));
+	return ret;
 }
 
 static int
@@ -813,7 +852,7 @@ const struct address_space_operations ex
 	.writepage		= ext2_writepage,
 	.sync_page		= block_sync_page,
 	.write_begin		= ext2_write_begin,
-	.write_end		= generic_write_end,
+	.write_end		= ext2_write_end,
 	.bmap			= ext2_bmap,
 	.direct_IO		= ext2_direct_IO,
 	.writepages		= ext2_writepages,
@@ -1022,7 +1061,7 @@ static void ext2_free_branches(struct in
 		ext2_free_data(inode, p, q);
 }
 
-void ext2_truncate(struct inode *inode)
+static void __ext2_truncate_blocks(struct inode *inode, loff_t offset)
 {
 	__le32 *i_data = EXT2_I(inode)->i_data;
 	struct ext2_inode_info *ei = EXT2_I(inode);
@@ -1034,27 +1073,8 @@ void ext2_truncate(struct inode *inode)
 	int n;
 	long iblock;
 	unsigned blocksize;
-
-	if (!(S_ISREG(inode->i_mode) || S_ISDIR(inode->i_mode) ||
-	    S_ISLNK(inode->i_mode)))
-		return;
-	if (ext2_inode_is_fast_symlink(inode))
-		return;
-	if (IS_APPEND(inode) || IS_IMMUTABLE(inode))
-		return;
-
 	blocksize = inode->i_sb->s_blocksize;
-	iblock = (inode->i_size + blocksize-1)
-					>> EXT2_BLOCK_SIZE_BITS(inode->i_sb);
-
-	if (mapping_is_xip(inode->i_mapping))
-		xip_truncate_page(inode->i_mapping, inode->i_size);
-	else if (test_opt(inode->i_sb, NOBH))
-		nobh_truncate_page(inode->i_mapping,
-				inode->i_size, ext2_get_block);
-	else
-		block_truncate_page(inode->i_mapping,
-				inode->i_size, ext2_get_block);
+	iblock = (offset + blocksize-1) >> EXT2_BLOCK_SIZE_BITS(inode->i_sb);
 
 	n = ext2_block_to_path(inode, iblock, offsets, NULL);
 	if (n == 0)
@@ -1122,6 +1142,62 @@ do_indirects:
 	ext2_discard_reservation(inode);
 
 	mutex_unlock(&ei->truncate_mutex);
+}
+
+static void ext2_truncate_blocks(struct inode *inode, loff_t offset)
+{
+	/*
+	 * XXX: it seems like a bug here that we don't allow
+	 * IS_APPEND inode to have blocks-past-i_size trimmed off.
+	 * review and fix this.
+	 *
+	 * Also would be nice to be able to handle IO errors and such,
+	 * but that's probably too much to ask.
+	 */
+	if (!(S_ISREG(inode->i_mode) || S_ISDIR(inode->i_mode) ||
+	    S_ISLNK(inode->i_mode)))
+		return;
+	if (ext2_inode_is_fast_symlink(inode))
+		return;
+	if (IS_APPEND(inode) || IS_IMMUTABLE(inode))
+		return;
+	__ext2_truncate_blocks(inode, offset);
+}
+
+int ext2_setsize(struct inode *inode, loff_t newsize)
+{
+	loff_t oldsize;
+	int error;
+
+	error = inode_newsize_ok(inode, newsize);
+	if (error)
+		return error;
+
+	if (!(S_ISREG(inode->i_mode) || S_ISDIR(inode->i_mode) ||
+	    S_ISLNK(inode->i_mode)))
+		return -EINVAL;
+	if (ext2_inode_is_fast_symlink(inode))
+		return -EINVAL;
+	if (IS_APPEND(inode) || IS_IMMUTABLE(inode))
+		return -EPERM;
+
+	if (mapping_is_xip(inode->i_mapping))
+		error = xip_truncate_page(inode->i_mapping, newsize);
+	else if (test_opt(inode->i_sb, NOBH))
+		error = nobh_truncate_page(inode->i_mapping,
+				newsize, ext2_get_block);
+	else
+		error = block_truncate_page(inode->i_mapping,
+				newsize, ext2_get_block);
+	if (error)
+		return error;
+
+	oldsize = inode->i_size;
+	i_size_write(inode, newsize);
+	truncate_pagecache(inode, oldsize, newsize);
+
+	__ext2_truncate_blocks(inode, newsize);
+
 	inode->i_mtime = inode->i_ctime = CURRENT_TIME_SEC;
 	if (inode_needs_sync(inode)) {
 		sync_mapping_buffers(inode->i_mapping);
@@ -1129,6 +1205,8 @@ do_indirects:
 	} else {
 		mark_inode_dirty(inode);
 	}
+
+	return 0;
 }
 
 static struct ext2_inode *ext2_get_inode(struct super_block *sb, ino_t ino,
@@ -1461,8 +1539,15 @@ int ext2_setattr(struct dentry *dentry,
 		if (error)
 			return error;
 	}
-	error = inode_setattr(inode, iattr);
-	if (!error && (iattr->ia_valid & ATTR_MODE))
+	if (iattr->ia_valid & ATTR_SIZE) {
+		error = ext2_setsize(inode, iattr->ia_size);
+		if (error)
+			return error;
+	}
+	generic_setattr(inode, iattr);
+	if (iattr->ia_valid & ATTR_MODE)
 		error = ext2_acl_chmod(inode);
+	mark_inode_dirty(inode);
+
 	return error;
 }

Re: [patch 4/5] ext2: convert to use the new truncate convention

[Jan Kara]

On Tue 08-12-09 09:42:09, Nick Piggin wrote:
> I also have commented a possible bug in existing ext2 code, marked with XXX.
> 
> Cc: linux-ext4@vger.kernel.org
> Cc: Christoph Hellwig <hch@lst.de>
> Signed-off-by: Nick Piggin <npiggin@suse.de>
...
> @@ -752,8 +764,8 @@ int __ext2_write_begin(struct file *file
>  		loff_t pos, unsigned len, unsigned flags,
>  		struct page **pagep, void **fsdata)
>  {
> -	return block_write_begin(file, mapping, pos, len, flags, pagep, fsdata,
> -							ext2_get_block);
> +	return block_write_begin_newtrunc(file, mapping, pos, len, flags,
> +					pagep, fsdata, ext2_get_block);
>  }
  OK, but you should update the code in dir.c using __ext2_write_begin,
shouldn't you?

> +static int ext2_write_end(struct file *file, struct address_space *mapping,
> +			loff_t pos, unsigned len, unsigned copied,
> +			struct page *page, void *fsdata)
> +{
> +	int ret;
> +
> +	ret = generic_write_end(file, mapping, pos, len, copied, page, fsdata);
> +	if (ret < len)
> +		ext2_write_failed(mapping, pos + len);
> +	return ret;
>  }
  OK, when doing this, please also update ext2_commit_chunk in dir.c...

> +static void ext2_truncate_blocks(struct inode *inode, loff_t offset)
> +{
> +	/*
> +	 * XXX: it seems like a bug here that we don't allow
> +	 * IS_APPEND inode to have blocks-past-i_size trimmed off.
> +	 * review and fix this.
> +	 *
> +	 * Also would be nice to be able to handle IO errors and such,
> +	 * but that's probably too much to ask.
> +	 */
> +	if (!(S_ISREG(inode->i_mode) || S_ISDIR(inode->i_mode) ||
> +	    S_ISLNK(inode->i_mode)))
> +		return;
> +	if (ext2_inode_is_fast_symlink(inode))
> +		return;
> +	if (IS_APPEND(inode) || IS_IMMUTABLE(inode))
> +		return;
  Yes, I'd remove IS_APPEND check from here.

								Honza
-- 
Jan Kara <jack@suse.cz>
SUSE Labs, CR

Re: [patch 4/5] ext2: convert to use the new truncate convention

[Nick Piggin]

On Wed, Dec 09, 2009 at 03:57:13PM +0100, Jan Kara wrote:
> On Tue 08-12-09 09:42:09, Nick Piggin wrote:
> > I also have commented a possible bug in existing ext2 code, marked with XXX.
> > 
> > Cc: linux-ext4@vger.kernel.org
> > Cc: Christoph Hellwig <hch@lst.de>
> > Signed-off-by: Nick Piggin <npiggin@suse.de>
> ...
> > @@ -752,8 +764,8 @@ int __ext2_write_begin(struct file *file
> >  		loff_t pos, unsigned len, unsigned flags,
> >  		struct page **pagep, void **fsdata)
> >  {
> > -	return block_write_begin(file, mapping, pos, len, flags, pagep, fsdata,
> > -							ext2_get_block);
> > +	return block_write_begin_newtrunc(file, mapping, pos, len, flags,
> > +					pagep, fsdata, ext2_get_block);
> >  }
>   OK, but you should update the code in dir.c using __ext2_write_begin,
> shouldn't you?

To trim off blocks past i_size on failure? Yes I guess it should.
In fact, the ext2_write_failed call could just go into here I think.
I'll take a look.


> > +static int ext2_write_end(struct file *file, struct address_space *mapping,
> > +			loff_t pos, unsigned len, unsigned copied,
> > +			struct page *page, void *fsdata)
> > +{
> > +	int ret;
> > +
> > +	ret = generic_write_end(file, mapping, pos, len, copied, page, fsdata);
> > +	if (ret < len)
> > +		ext2_write_failed(mapping, pos + len);
> > +	return ret;
> >  }
>   OK, when doing this, please also update ext2_commit_chunk in dir.c...

I think commit_chunk is OK. Because block_write_end does not fail
and the only reason for checking failure here is in case of a short
copy (which won't happen in dir.c code).

 
> > +static void ext2_truncate_blocks(struct inode *inode, loff_t offset)
> > +{
> > +	/*
> > +	 * XXX: it seems like a bug here that we don't allow
> > +	 * IS_APPEND inode to have blocks-past-i_size trimmed off.
> > +	 * review and fix this.
> > +	 *
> > +	 * Also would be nice to be able to handle IO errors and such,
> > +	 * but that's probably too much to ask.
> > +	 */
> > +	if (!(S_ISREG(inode->i_mode) || S_ISDIR(inode->i_mode) ||
> > +	    S_ISLNK(inode->i_mode)))
> > +		return;
> > +	if (ext2_inode_is_fast_symlink(inode))
> > +		return;
> > +	if (IS_APPEND(inode) || IS_IMMUTABLE(inode))
> > +		return;
>   Yes, I'd remove IS_APPEND check from here.

Didn't want to change that in this patch, but I'm happy for someone to
fix it (and in ext3/4 etc).

The checks probably should be done at a different level anyway: by the
time that this code gets called, the pagecache is truncated and i_size
modified anyway so it seems buggy if this made any difference.

Re: [patch 4/5] ext2: convert to use the new truncate convention

[Jan Kara]

On Thu 10-12-09 12:11:03, Nick Piggin wrote:
> On Wed, Dec 09, 2009 at 03:57:13PM +0100, Jan Kara wrote:
> > On Tue 08-12-09 09:42:09, Nick Piggin wrote:
> > > I also have commented a possible bug in existing ext2 code, marked with XXX.
> > > 
> > > Cc: linux-ext4@vger.kernel.org
> > > Cc: Christoph Hellwig <hch@lst.de>
> > > Signed-off-by: Nick Piggin <npiggin@suse.de>
> > ...
> > > @@ -752,8 +764,8 @@ int __ext2_write_begin(struct file *file
> > >  		loff_t pos, unsigned len, unsigned flags,
> > >  		struct page **pagep, void **fsdata)
> > >  {
> > > -	return block_write_begin(file, mapping, pos, len, flags, pagep, fsdata,
> > > -							ext2_get_block);
> > > +	return block_write_begin_newtrunc(file, mapping, pos, len, flags,
> > > +					pagep, fsdata, ext2_get_block);
> > >  }
> >   OK, but you should update the code in dir.c using __ext2_write_begin,
> > shouldn't you?
> 
> To trim off blocks past i_size on failure? Yes I guess it should.
> In fact, the ext2_write_failed call could just go into here I think.
> I'll take a look.
  Yes, that would be probably the easiest.

> > > +static int ext2_write_end(struct file *file, struct address_space *mapping,
> > > +			loff_t pos, unsigned len, unsigned copied,
> > > +			struct page *page, void *fsdata)
> > > +{
> > > +	int ret;
> > > +
> > > +	ret = generic_write_end(file, mapping, pos, len, copied, page, fsdata);
> > > +	if (ret < len)
> > > +		ext2_write_failed(mapping, pos + len);
> > > +	return ret;
> > >  }
> >   OK, when doing this, please also update ext2_commit_chunk in dir.c...
> 
> I think commit_chunk is OK. Because block_write_end does not fail
> and the only reason for checking failure here is in case of a short
> copy (which won't happen in dir.c code).
  Ah, right.

> > > +static void ext2_truncate_blocks(struct inode *inode, loff_t offset)
> > > +{
> > > +	/*
> > > +	 * XXX: it seems like a bug here that we don't allow
> > > +	 * IS_APPEND inode to have blocks-past-i_size trimmed off.
> > > +	 * review and fix this.
> > > +	 *
> > > +	 * Also would be nice to be able to handle IO errors and such,
> > > +	 * but that's probably too much to ask.
> > > +	 */
> > > +	if (!(S_ISREG(inode->i_mode) || S_ISDIR(inode->i_mode) ||
> > > +	    S_ISLNK(inode->i_mode)))
> > > +		return;
> > > +	if (ext2_inode_is_fast_symlink(inode))
> > > +		return;
> > > +	if (IS_APPEND(inode) || IS_IMMUTABLE(inode))
> > > +		return;
> >   Yes, I'd remove IS_APPEND check from here.
> 
> Didn't want to change that in this patch, but I'm happy for someone to
> fix it (and in ext3/4 etc).
  OK.

> The checks probably should be done at a different level anyway: by the
> time that this code gets called, the pagecache is truncated and i_size
> modified anyway so it seems buggy if this made any difference.
  It depends. The first if can be WARN_ON as well - we shouldn't really get
different inode types here. For fast symlinks we have nothing to truncate
so we want to immediately return doing nothing. For IMMUTABLE inode it is
again a bug if we get to this function and for APPEND inode the only
acceptable way to get here is the error recovery. We can clean that up
when your patch series goes in.

									Honza
-- 
Jan Kara <jack@suse.cz>
SUSE Labs, CR