exofs: New truncate sequence

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 <[email protected]>

CC Nick Piggin <[email protected]>

Signed-off-by Boaz Harrosh <[email protected]>

---
 fs/exofs/exofs.h |    1 -
 fs/exofs/file.c  |    1 -
 fs/exofs/inode.c |  115 +++++++++++++++++++++++++++---------------------------

 3 files changed, 58 insertions(+), 59 deletions(-)

fs/exofs/exofs.h +0 -1

index 22721b2..0706ce9 100644
@@ -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,

fs/exofs/file.c +0 -1

index fef6899..f9bfe2b 100644
@@ -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,
 };

fs/exofs/inode.c +1 -1

index 4bb6ef8..1c2666c 100644
@@ -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");
 		}

fs/exofs/inode.c +0 -0

@@ -1335,28 +1339,25 @@ void exofs_delete_inode(struct inode *inode)
 
 	truncate_inode_pages(&inode->i_data, 0);
 

fs/exofs/inode.c +8 -7

-
 	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);

Signature · 1 line

1.6.6.1

Quoted 30 lines · Boaz Harrosh

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?

Quoted 7 lines

CC Christoph Hellwig <[email protected]>

CC Nick Piggin <[email protected]>

Signed-off-by Boaz Harrosh <[email protected]>

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

Quoted 48 lines

 3 files changed, 58 insertions(+), 59 deletions(-)

fs/exofs/exofs.h +0 -1

index 22721b2..0706ce9 100644
@@ -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,

fs/exofs/file.c +0 -1

index fef6899..f9bfe2b 100644
@@ -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,
 };

fs/exofs/inode.c +5 -1

index 4bb6ef8..1c2666c 100644
@@ -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;
 

fs/exofs/inode.c int exofs_write_begin(struct file *file, struct address_space *mapping, · line 736

Quoted 4 lines

fs/exofs/inode.c +4 -0

+		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?

Quoted 9 lines

fs/exofs/inode.c +2 -0

+		return ret;
+	}
 	return ret;
 }
 

@@ -750,6 +760,10 @@ static int exofs_write_end(struct file *file, struct address_space *mapping,
 	int ret;
 

fs/exofs/inode.c static int exofs_write_end(struct file *file, struct address_space *mapping, · line 765

Quoted 4 lines

fs/exofs/inode.c +3 -0

 	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?

fs/exofs/inode.c static int exofs_write_end(struct file *file, struct address_space *mapping, · line 769

Quoted 5 lines

fs/exofs/inode.c +1 -0

+	/* 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?

Quoted 9 lines

fs/exofs/inode.c +1 -0

@@ -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;
 

fs/exofs/inode.c void exofs_delete_inode(struct inode *inode) · line 1346

Quoted 6 lines

fs/exofs/inode.c +0 -5

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

Quoted 24 lines

fs/exofs/inode.c +8 -7

-
 	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;
 	}
 

Quoted 3 lines

fs/exofs/inode.c +0 -0

 	ios->obj.id = exofs_oi_objno(oi);

Signature · 1 line

1.6.6.1

Quoted 6 lines · Nick Piggin

Quoted 4 lines · Boaz Harrosh

---
 fs/exofs/exofs.h |    1 -
 fs/exofs/file.c  |    1 -
 fs/exofs/inode.c |  115 +++++++++++++++++++++++++++---------------------------

Quoted 2 lines

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

I hope I don't have any left, that was the last, have I missed something?

Quoted 11 lines

fs/exofs/inode.c static int exofs_write_end(struct file *file, struct address_space *mapping, · line 765

Quoted 7 lines

fs/exofs/inode.c +3 -0

@@ -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?

No blocks are setup. Exofs does not have any blocks.

If the write
failed then the object on OSD has the last written offset as objects size. If error handling was done right residual is subtracted from IO size and should be reflected here.

If IO was never attempted then object size did not grow and we revert in memory i_size here. (size is keept in two places and is checked for consistency in fsck)

TODO:
eject short writes and see if this works correctly.

Quoted 8 lines

fs/exofs/inode.c static int exofs_write_end(struct file *file, struct address_space *mapping, · line 769

Quoted 5 lines

fs/exofs/inode.c +1 -0

+	/* 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?

I think Christoph has a patch for that.

Quoted 9 lines

Quoted 9 lines

fs/exofs/inode.c +1 -0

@@ -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;
 

Quoted 10 lines

fs/exofs/inode.c void exofs_delete_inode(struct inode *inode) · line 1346

Quoted 6 lines

fs/exofs/inode.c +0 -5

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

You see this is where exofs is different a file is an object_no on multiple OSD devices. The inode is kept as an attribute of the object. (data as object's data) so a exofs_sbi_remove will just obliterate any association to the object. It was historically called because exofs_truncate used to do what truncate_inode_pages does today. (And some other in memory book keeping.) But with your help all this was cleaned up.

Do you see any operation I missed that might need cleaning from the generic VFS inode, that might now leak. As far as storage is concerned I'm covered.

[I ran git clone linux; rm -rf linux; 100 times in a loop and the OSD
storage stayed constant size. So I presume there is no storage leak. OSD is good in this respect]

Quoted 23 lines

Quoted 23 lines

fs/exofs/inode.c +8 -7

-
 	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;
 	}
 

Quoted 3 lines

Quoted 3 lines

fs/exofs/inode.c +0 -0

 	ios->obj.id = exofs_oi_objno(oi);

Signature · 1 line

1.6.6.1

Thanks for lookin. And thanks for making this patch possible. I wanted this cleaned, long ago, but it was only made easy and simple after your changes.

Quoted 10 lines · Boaz Harrosh

Quoted 8 lines · Nick Piggin

Quoted 4 lines · Boaz Harrosh

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

Quoted 2 lines

I hope I don't have any left, that was the last, have I missed something?

exofs_invalidatepage, exofs_releasepage, includes of buffer_head.h. No point to any of that if you never actually map the buffers or use them for tracking state yourself.

Quoted 8 lines

You see this is where exofs is different a file is an object_no on multiple OSD devices. The inode is kept as an attribute of the object. (data as object's data) so a exofs_sbi_remove will just obliterate any association to the object. It was historically called because exofs_truncate used to do what truncate_inode_pages does today. (And some other in memory book keeping.) But with your help all this was cleaned up.

OK, I was thinking the underlying object itself needs to be trimmed to match i_size similarly to just a block based filesystem? Like exofs_oi_truncate appears to.

Quoted 5 lines

Do you see any operation I missed that might need cleaning from the generic VFS inode, that might now leak. As far as storage is concerned I'm covered.

Quoted 3 lines

[I ran git clone linux; rm -rf linux; 100 times in a loop and the OSD
storage stayed constant size. So I presume there is no storage leak. OSD is good in this respect]

I can't see anything off hand. Was just flagging points where vmtruncate or truncate had been called and is not now. If you have all those covered, then you should be OK.

Quoted 14 lines · Nick Piggin

Quoted 12 lines · Boaz Harrosh

Quoted 8 lines · Nick Piggin

Quoted 4 lines · Boaz Harrosh

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

I hope I don't have any left, that was the last, have I missed something?

Quoted 4 lines

exofs_invalidatepage, exofs_releasepage, includes of buffer_head.h. No point to any of that if you never actually map the buffers or use them for tracking state yourself.

Rrr thanks. Yes I'll leave the WARN_ON and do nothing, and remove the include. I'll submit for linux-next.

Thanks.

<snip>

Quoted 9 lines

Quoted 8 lines

You see this is where exofs is different a file is an object_no on multiple OSD devices. The inode is kept as an attribute of the object. (data as object's data) so a exofs_sbi_remove will just obliterate any association to the object. It was historically called because exofs_truncate used to do what truncate_inode_pages does today. (And some other in memory book keeping.) But with your help all this was cleaned up.

Quoted 4 lines

OK, I was thinking the underlying object itself needs to be trimmed to match i_size similarly to just a block based filesystem? Like exofs_oi_truncate appears to.

Yes you are right, generally, but since I'm doing exofs_sbi_remove() just after that then there is no point. the osd_remove will take care of that anyway.

Quoted 4 lines

Quoted 4 lines

Do you see any operation I missed that might need cleaning from the generic VFS inode, that might now leak. As far as storage is concerned I'm covered.

Quoted 9 lines

Quoted 3 lines

[I ran git clone linux; rm -rf linux; 100 times in a loop and the OSD
storage stayed constant size. So I presume there is no storage leak. OSD is good in this respect]

I can't see anything off hand. Was just flagging points where vmtruncate or truncate had been called and is not now. If you have all those covered, then you should be OK.

I'll be testing this particular branch for a while. I have a chicken and egg problem. I think I'm kind of dependent on Al's vfs for-next branch. Do you think the patch could also work with Linus-2.6.35-rc1? I'm afraid that even if so It might conflict with Al's tree if I put it independently in the osd/for-next tree.

Quoted 20 lines · Boaz Harrosh

Quoted 18 lines · Nick Piggin

Quoted 7 lines · Boaz Harrosh

Quoted 6 lines · Nick Piggin

Quoted 4 lines · Boaz Harrosh

---
 fs/exofs/exofs.h |    1 -
 fs/exofs/file.c  |    1 -
 fs/exofs/inode.c |  115 +++++++++++++++++++++++++++---------------------------

Quoted 5 lines

Quoted 2 lines

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

I hope I don't have any left, that was the last, have I missed something?

exofs_invalidatepage, exofs_releasepage, includes of buffer_head.h. No point to any of that if you never actually map the buffers or use them for tracking state yourself.

Quoted 2 lines

Rrr thanks. Yes I'll leave the WARN_ON and do nothing, and remove the include. I'll submit for linux-next.

OK. I'd get rid of it completely before it hits mainline.

I would keep them based on top of Christoph's patches (just cherry pick the exofs hunks in the meantime).

You mean pick them from the mailing-list. But then there is no chance for them to go into linux-next and pick up some testing. That's why I thought on rebasing on Linus-2.6.35-rc1.

Then when Christoph's patches get picked by Al I'll rebase. But the base is just good right now and I'd like some exposure for this.

Christoph this is your call

Quoted 4 lines · Boaz Harrosh

You mean pick them from the mailing-list. But then there is no chance for them to go into linux-next and pick up some testing. That's why I thought on rebasing on Linus-2.6.35-rc1.

Quoted 4 lines

Then when Christoph's patches get picked by Al I'll rebase. But the base is just good right now and I'd like some exposure for this.

Christoph this is your call

In the end it really depends on Al. If you want to push your patches out ASAP feel free to do it and I can rebase easily.

Imagine my surprise when this morning I see that all of Al's patches are in mainline. (that was sarcastic, Stupid me they where there yesterday)

So I'm posting a final version of this patch. It runs over 2.6.35-rc1 and works really well and I like this patch a lot. I'm putting it in osd/linux-next.

Please advise: Should I ask Linus to pull this in for current Kernel? If not, then I'll ask Al to put it in his tree for 2.6.36 and you can rebase over his tree.

Quoted 7 lines · Boaz Harrosh

Imagine my surprise when this morning I see that all of Al's patches are in mainline. (that was sarcastic, Stupid me they where there yesterday)

So I'm posting a final version of this patch. It runs over 2.6.35-rc1 and works really well and I like this patch a lot. I'm putting it in osd/linux-next.

Quoted 3 lines

Please advise: Should I ask Linus to pull this in for current Kernel? If not, then I'll ask Al to put it in his tree for 2.6.36 and you can rebase over his tree.

With the other changes I have you'll be much better off rebasing ontop of my series as it will make your life easier. But if you're eager to push it out please do it ASAP.

I would prefer to push it, because it will be much easier for me when juggling with the pnfs tree, Which is based on 2.6.35-rc1.

fs/exofs/inode.c static int _do_truncate(struct inode *inode) · line 854

Quoted 6 lines · Boaz Harrosh

fs/exofs/inode.c +4 -1

+	if (!(S_ISREG(inode->i_mode) || S_ISDIR(inode->i_mode) ||
+	    S_ISLNK(inode->i_mode)))
+		return -EINVAL;
 	if (exofs_inode_is_fast_symlink(inode))
-		return;
+		return -EINVAL;

do_sys_truncate already makes sure ATTR_SIZE changes only happen on ISREG files.

Review

Quoted 2 lines

(quoted diff) +0 -1

 	if (IS_APPEND(inode) || IS_IMMUTABLE(inode))
-		return;

Same for these - IS_APPEND directly and IS_IMMUTABLE via inode_permission().

fs/exofs/inode.c static int _do_truncate(struct inode *inode) · line 860

Quoted 3 lines

fs/exofs/inode.c +3 -0

+	oldsize = inode->i_size;
+	i_size_write(inode, newsize);
+	truncate_pagecache(inode, oldsize, newsize);

At this point you can probably just inline this in exofs_setattr. Especially as the lines above will be replaced with a single function call in my next batch.

I copy/pasted this from ext2_setsize. (Would you remove it from there as well?)

Quoted 5 lines · Boaz Harrosh

fs/exofs/inode.c +1 -0

+static int _do_truncate(struct inode *inode, loff_t newsize)
 {
 	struct exofs_i_info *oi = exofs_i(inode);
 	int ret;

fs/exofs/inode.c static inline int exofs_inode_is_fast_symlink(struct inode *inode) · line 834

Quoted 5 lines

fs/exofs/inode.c +2 -0

 	inode->i_mtime = inode->i_ctime = CURRENT_TIME;
 
+	ret = exofs_oi_truncate(oi, (u64)newsize);
+	EXOFS_DBGMSG("(0x%lx) size=0x%llx\n", inode->i_ino, newsize);
 	return ret;

Maybe I'm beeing picky, but I really don't see the need for this function either, especially given that is has a single caller.

These changes are crafted based on the similar
conversion done to ext2 by Nick Piggin.

* Remove the deprecated ->truncate vector. Let exofs_setattr take care of on-disk size updates.
* 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 need a partial read here upto i_size ???]