diff mbox

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

Message ID 20090816102856.846088617@suse.de
State Not Applicable, archived
Headers show

Commit Message

Nick Piggin Aug. 16, 2009, 10:25 a.m. UTC 
I also have some questions, 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  |    2 
 fs/ext2/inode.c |  138 +++++++++++++++++++++++++++++++++++++++++++++-----------
 3 files changed, 113 insertions(+), 28 deletions(-)



--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Christoph Hellwig Aug. 16, 2009, 8:16 p.m. UTC | #1 
On Sun, Aug 16, 2009 at 08:25:38PM +1000, npiggin@suse.de wrote:
> @@ -66,9 +68,10 @@ void ext2_delete_inode (struct inode * i
>  	mark_inode_dirty(inode);
>  	ext2_write_inode(inode, inode_needs_sync(inode));
>  
> +	/* XXX: where is truncate_inode_pages? */
>  	inode->i_size = 0;
>  	if (inode->i_blocks)
> -		ext2_truncate (inode);
> +		ext2_truncate_blocks(inode, 0);
>  	ext2_free_inode (inode);

At the beginning of the function, just before the diff window.  Because
this is ->delete_inode we truncate away all pages, down to offset 0.

> +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.
> +	 */
> +	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;
> +	__ext2_truncate_blocks(inode, offset);

Yes, I think the IS_APPEND(inode) || IS_IMMUTABLE(inode) checks should move
into ext2_setsize.  But let's leave that for a separate patch.

Btw, the above code gives me warnings like this:

/home/hch/work/linux-2.6/fs/ext2/inode.c: In function
'ext2_truncate_blocks':
/home/hch/work/linux-2.6/fs/ext2/inode.c:1158: warning: 'return' with a
value, in function returning void
/home/hch/work/linux-2.6/fs/ext2/inode.c:1160: warning: 'return' with a
value, in function returning void
/home/hch/work/linux-2.6/fs/ext2/inode.c:1162: warning: 'return' with a
value, in function returning void

because you try to return errors from a function delcared as void.
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Nick Piggin Aug. 17, 2009, 6:42 a.m. UTC | #2 
On Sun, Aug 16, 2009 at 04:16:26PM -0400, Christoph Hellwig wrote:
> On Sun, Aug 16, 2009 at 08:25:38PM +1000, npiggin@suse.de wrote:
> > @@ -66,9 +68,10 @@ void ext2_delete_inode (struct inode * i
> >  	mark_inode_dirty(inode);
> >  	ext2_write_inode(inode, inode_needs_sync(inode));
> >  
> > +	/* XXX: where is truncate_inode_pages? */
> >  	inode->i_size = 0;
> >  	if (inode->i_blocks)
> > -		ext2_truncate (inode);
> > +		ext2_truncate_blocks(inode, 0);
> >  	ext2_free_inode (inode);
> 
> At the beginning of the function, just before the diff window.  Because
> this is ->delete_inode we truncate away all pages, down to offset 0.

OK, weird. I thought I couldn't see it when I wrote that :) maybe my
tree was corrupted or I'm stupid.


> > +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.
> > +	 */
> > +	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;
> > +	__ext2_truncate_blocks(inode, offset);
> 
> Yes, I think the IS_APPEND(inode) || IS_IMMUTABLE(inode) checks should move
> into ext2_setsize.  But let's leave that for a separate patch.

Yeah agreed.

 
> Btw, the above code gives me warnings like this:
> 
> /home/hch/work/linux-2.6/fs/ext2/inode.c: In function
> 'ext2_truncate_blocks':
> /home/hch/work/linux-2.6/fs/ext2/inode.c:1158: warning: 'return' with a
> value, in function returning void
> /home/hch/work/linux-2.6/fs/ext2/inode.c:1160: warning: 'return' with a
> value, in function returning void
> /home/hch/work/linux-2.6/fs/ext2/inode.c:1162: warning: 'return' with a
> value, in function returning void
> 
> because you try to return errors from a function delcared as void.

Hm, sorry. I thought it was in good shape... I'll recheck that I sent
out the correct versions and resend according to feedback from you
and Hugh.

Thanks,
Nick

--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Boaz Harrosh Aug. 17, 2009, 11:09 a.m. UTC | #3 
On 08/17/2009 09:42 AM, Nick Piggin wrote:
> On Sun, Aug 16, 2009 at 04:16:26PM -0400, Christoph Hellwig wrote:
>> On Sun, Aug 16, 2009 at 08:25:38PM +1000, npiggin@suse.de wrote:
>>> @@ -66,9 +68,10 @@ void ext2_delete_inode (struct inode * i
>>>  	mark_inode_dirty(inode);
>>>  	ext2_write_inode(inode, inode_needs_sync(inode));
>>>  
>>> +	/* XXX: where is truncate_inode_pages? */
>>>  	inode->i_size = 0;
>>>  	if (inode->i_blocks)
>>> -		ext2_truncate (inode);
>>> +		ext2_truncate_blocks(inode, 0);
>>>  	ext2_free_inode (inode);
>>
>> At the beginning of the function, just before the diff window.  Because
>> this is ->delete_inode we truncate away all pages, down to offset 0.
> 
> OK, weird. I thought I couldn't see it when I wrote that :) maybe my
> tree was corrupted or I'm stupid.
> 
> 
>>> +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.
>>> +	 */
>>> +	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;
>>> +	__ext2_truncate_blocks(inode, offset);
>>
>> Yes, I think the IS_APPEND(inode) || IS_IMMUTABLE(inode) checks should move
>> into ext2_setsize.  But let's leave that for a separate patch.
> 
> Yeah agreed.
> 
>  
>> Btw, the above code gives me warnings like this:
>>
>> /home/hch/work/linux-2.6/fs/ext2/inode.c: In function
>> 'ext2_truncate_blocks':
>> /home/hch/work/linux-2.6/fs/ext2/inode.c:1158: warning: 'return' with a
>> value, in function returning void
>> /home/hch/work/linux-2.6/fs/ext2/inode.c:1160: warning: 'return' with a
>> value, in function returning void
>> /home/hch/work/linux-2.6/fs/ext2/inode.c:1162: warning: 'return' with a
>> value, in function returning void
>>
>> because you try to return errors from a function delcared as void.
> 
> Hm, sorry. I thought it was in good shape... I'll recheck that I sent
> out the correct versions and resend according to feedback from you
> and Hugh.
> 
> Thanks,
> Nick
> 

Nick do you have a public tree with these latest patches? I would like to
try out an exofs conversion and testing.

Thanks
Boaz
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Nick Piggin Aug. 17, 2009, 4:44 p.m. UTC | #4 
On Mon, Aug 17, 2009 at 02:09:54PM +0300, Boaz Harrosh wrote:
> On 08/17/2009 09:42 AM, Nick Piggin wrote:
> > On Sun, Aug 16, 2009 at 04:16:26PM -0400, Christoph Hellwig wrote:
> >> On Sun, Aug 16, 2009 at 08:25:38PM +1000, npiggin@suse.de wrote:
> >>> @@ -66,9 +68,10 @@ void ext2_delete_inode (struct inode * i
> >>>  	mark_inode_dirty(inode);
> >>>  	ext2_write_inode(inode, inode_needs_sync(inode));
> >>>  
> >>> +	/* XXX: where is truncate_inode_pages? */
> >>>  	inode->i_size = 0;
> >>>  	if (inode->i_blocks)
> >>> -		ext2_truncate (inode);
> >>> +		ext2_truncate_blocks(inode, 0);
> >>>  	ext2_free_inode (inode);
> >>
> >> At the beginning of the function, just before the diff window.  Because
> >> this is ->delete_inode we truncate away all pages, down to offset 0.
> > 
> > OK, weird. I thought I couldn't see it when I wrote that :) maybe my
> > tree was corrupted or I'm stupid.
> > 
> > 
> >>> +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.
> >>> +	 */
> >>> +	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;
> >>> +	__ext2_truncate_blocks(inode, offset);
> >>
> >> Yes, I think the IS_APPEND(inode) || IS_IMMUTABLE(inode) checks should move
> >> into ext2_setsize.  But let's leave that for a separate patch.
> > 
> > Yeah agreed.
> > 
> >  
> >> Btw, the above code gives me warnings like this:
> >>
> >> /home/hch/work/linux-2.6/fs/ext2/inode.c: In function
> >> 'ext2_truncate_blocks':
> >> /home/hch/work/linux-2.6/fs/ext2/inode.c:1158: warning: 'return' with a
> >> value, in function returning void
> >> /home/hch/work/linux-2.6/fs/ext2/inode.c:1160: warning: 'return' with a
> >> value, in function returning void
> >> /home/hch/work/linux-2.6/fs/ext2/inode.c:1162: warning: 'return' with a
> >> value, in function returning void
> >>
> >> because you try to return errors from a function delcared as void.
> > 
> > Hm, sorry. I thought it was in good shape... I'll recheck that I sent
> > out the correct versions and resend according to feedback from you
> > and Hugh.
> > 
> > Thanks,
> > Nick
> > 
> 
> Nick do you have a public tree with these latest patches? I would like to
> try out an exofs conversion and testing.

Hi Boaz,

I don't have a public tree. I can send you a rollup if you ping me or
just take the patches from the list. I will send out a new set shortly
(with very minor differences -- a couple of new helper functions to
use).

That will be great if you can convert your fs. I will really appreciate
it.

Thanks,
Nick
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

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,8 @@  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);
+
 /*
  * Called at the last iput() if i_nlink is zero.
  */
@@ -66,9 +68,10 @@  void ext2_delete_inode (struct inode * i
 	mark_inode_dirty(inode);
 	ext2_write_inode(inode, inode_needs_sync(inode));
 
+	/* XXX: where is truncate_inode_pages? */
 	inode->i_size = 0;
 	if (inode->i_blocks)
-		ext2_truncate (inode);
+		ext2_truncate_blocks(inode, 0);
 	ext2_free_inode (inode);
 
 	return;
@@ -761,8 +764,33 @@  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) {
+		struct inode *inode = mapping->host;
+		loff_t isize = inode->i_size;
+		if (pos + len > isize)
+			ext2_truncate_blocks(inode, isize);
+	}
+	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) {
+		struct inode *inode = mapping->host;
+		loff_t isize = inode->i_size;
+		if (pos + len > isize)
+			ext2_truncate_blocks(inode, isize);
+	}
+	return ret;
 }
 
 static int
@@ -770,13 +798,22 @@  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,
+	ret = nobh_write_begin(file, mapping, pos, len, flags, pagep, fsdata,
 							ext2_get_block);
+	if (ret < 0) {
+		struct inode *inode = mapping->host;
+		loff_t isize = inode->i_size;
+		if (pos + len > isize)
+			ext2_truncate_blocks(inode, isize);
+	}
+	return ret;
 }
 
 static int ext2_nobh_writepage(struct page *page,
@@ -796,9 +833,15 @@  ext2_direct_IO(int rw, struct kiocb *ioc
 {
 	struct file *file = iocb->ki_filp;
 	struct inode *inode = file->f_mapping->host;
+	ssize_t ret;
 
-	return blockdev_direct_IO(rw, iocb, inode, inode->i_sb->s_bdev, iov,
+	ret = blockdev_direct_IO(rw, iocb, inode, inode->i_sb->s_bdev, iov,
 				offset, nr_segs, ext2_get_block, NULL);
+	if (ret < 0 && (rw & WRITE)) {
+		loff_t isize = inode->i_size;
+		ext2_truncate_blocks(inode, isize);
+	}
+	return ret;
 }
 
 static int
@@ -813,7 +856,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,
@@ -1020,7 +1063,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);
@@ -1032,27 +1075,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)
@@ -1120,6 +1144,59 @@  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.
+	 */
+	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;
+	__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);
@@ -1127,6 +1204,8 @@  do_indirects:
 	} else {
 		mark_inode_dirty(inode);
 	}
+
+	return 0;
 }
 
 static struct ext2_inode *ext2_get_inode(struct super_block *sb, ino_t ino,
@@ -1459,6 +1538,13 @@  int ext2_setattr(struct dentry *dentry,
 		if (error)
 			return error;
 	}
+	if (iattr->ia_valid & ATTR_SIZE) {
+		error = ext2_setsize(inode, iattr->ia_size);
+		if (error)
+			return error;
+		iattr->ia_valid &= ~ATTR_SIZE;
+		/* strip ATTR_SIZE for inode_setattr */
+	}
 	error = inode_setattr(inode, iattr);
 	if (!error && (iattr->ia_valid & ATTR_MODE))
 		error = ext2_acl_chmod(inode);
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,13 +77,13 @@  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,
 	.listxattr	= ext2_listxattr,
 	.removexattr	= generic_removexattr,
 #endif
+	.new_truncate	= 1,
 	.setattr	= ext2_setattr,
 	.permission	= ext2_permission,
 	.fiemap		= ext2_fiemap,