Re: ext2_discard_prealloc() called on each iput?
On Tue 29-05-07 14:10:52, Mingming Cao wrote: On Mon, 2007-05-28 at 18:04 +0200, Jan Kara wrote: On Wed 23-05-07 08:37:43, Theodore Tso wrote: On Tue, May 22, 2007 at 06:11:27PM +0200, Jan Kara wrote: while fixing some problems with preallocation in UDF, I had a look how ext2 solves similar problems. I found out that ext2_discard_prealloc() is called on every iput() from ext2_put_inode(). Is it really appropriate? I don't see a reason for doing so... I agree, it's probably not appropriate. It's been that way for a long time, though (since 2.4.20). It's not as horrible as it seems since unlike traditional Unix systems, we don't call iput() as often, since for example operations like close() end up calling dput(), which decrements the ref. count on dentry, not the inode. But it would probably be better to check to see if i_count is 1 before deciding to discard the preallocation. OK, but then you could move the code to drop_inode() which is called at exactly that moment... I've been thinking more about it when fixing UDF. I have tried to optimize ext2 discard preallocation code like ext3 discard reservation a while back: we only call ext2_discard_prealloc on the last iput(), i.e. ext2/3_clear_inode(). This patch actually made into mainline for a little while, then later it is being reversed back because of possible leak of preallocated blocks. Tt the unmount time, someone might still hold the reference of the inode, thus ext2_discard_prealloc() did not get a chance to be called. Since ext2 preallocation is doing pre-allocation on disk, this leads to leak of preallocated blocks, confused fsck later. http://lkml.org/lkml/2005/4/12/333 Interesting. I don't quite understand how it can happen that inode is not released at umount time... It must be released for umount to succeed. There is a slight problem that inode is not written after calling clear_inode() which could cause some problems (i_blocks being wrong) but blocks in bitmaps should be freed just right... anyway, so the preallocated region is less useful. OK, but still we could use e.g. i_writecount to check that we drop the last descriptor for writing... Yep, that is what ext3 does in ext3_release_file(), I forget why we didn't fix this for ext2. Hmm, probably we just forgot... Honza -- Jan Kara [EMAIL PROTECTED] SuSE CR Labs - To unsubscribe from this list: send the line unsubscribe linux-ext4 in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: ext2_discard_prealloc() called on each iput?
On Tue, May 29, 2007 at 12:25:45PM +0200, Jan Kara wrote:
Fix a comment when ext2_release_file() is called.
Signed-off-by: Jan Kara [EMAIL PROTECTED]
Acked-by: Theodore Ts'o [EMAIL PROTECTED]
diff -rupX /home/jack/.kerndiffexclude linux-2.6.21/fs/ext2/file.c
linux-2.6.21-1-ext2_comment_fix/fs/ext2/file.c
--- linux-2.6.21/fs/ext2/file.c 2007-05-15 14:18:47.0 +0200
+++ linux-2.6.21-1-ext2_comment_fix/fs/ext2/file.c2007-05-29
12:15:59.0 +0200
@@ -24,9 +24,9 @@
#include acl.h
/*
- * Called when an inode is released. Note that this is different
- * from ext2_open_file: open gets called at every open, but release
- * gets called only when /all/ the files are closed.
+ * Called when filp is released. This happens when all file descriptors
+ * for a single struct file are closed. Note that different open() calls
+ * for the same file yield different struct file structures.
*/
static int ext2_release_file (struct inode * inode, struct file * filp)
{
-
To unsubscribe from this list: send the line unsubscribe linux-ext4 in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: ext2_discard_prealloc() called on each iput?
On Mon, 2007-05-28 at 18:04 +0200, Jan Kara wrote: On Wed 23-05-07 08:37:43, Theodore Tso wrote: On Tue, May 22, 2007 at 06:11:27PM +0200, Jan Kara wrote: while fixing some problems with preallocation in UDF, I had a look how ext2 solves similar problems. I found out that ext2_discard_prealloc() is called on every iput() from ext2_put_inode(). Is it really appropriate? I don't see a reason for doing so... I agree, it's probably not appropriate. It's been that way for a long time, though (since 2.4.20). It's not as horrible as it seems since unlike traditional Unix systems, we don't call iput() as often, since for example operations like close() end up calling dput(), which decrements the ref. count on dentry, not the inode. But it would probably be better to check to see if i_count is 1 before deciding to discard the preallocation. OK, but then you could move the code to drop_inode() which is called at exactly that moment... I've been thinking more about it when fixing UDF. I have tried to optimize ext2 discard preallocation code like ext3 discard reservation a while back: we only call ext2_discard_prealloc on the last iput(), i.e. ext2/3_clear_inode(). This patch actually made into mainline for a little while, then later it is being reversed back because of possible leak of preallocated blocks. Tt the unmount time, someone might still hold the reference of the inode, thus ext2_discard_prealloc() did not get a chance to be called. Since ext2 preallocation is doing pre-allocation on disk, this leads to leak of preallocated blocks, confused fsck later. http://lkml.org/lkml/2005/4/12/333 Discarding prealloc at drop_inode() has the disadvantage that symlinks/directories will keep their preallocated blocks until inodes are evicted from memory. Which is probably why ext2 discards prealloc on iput(). Also I found slightly misleading the comment at ext2_release_file(). As far as I understand the code it isn't when /all/ files are closed but rather when all fd's for given filp are closed. I.e. if you open the same file two times, -release will get called once for each open. Am I right? Yep! If so, then also calling ext2_discard_prealloc() from ext2_release_file() is suboptimal, isn't it? Yes, although it's a bit better because only discaord the preallocation if the file descriptor was opened for writing. The file could be opened for writing by multiple file descriptors, true, but in that case it's likely that the write pattern will be a random access one anyway, so the preallocated region is less useful. OK, but still we could use e.g. i_writecount to check that we drop the last descriptor for writing... Yep, that is what ext3 does in ext3_release_file(), I forget why we didn't fix this for ext2. Mingming - To unsubscribe from this list: send the line unsubscribe linux-ext4 in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: ext2_discard_prealloc() called on each iput?
On Wed 23-05-07 08:37:43, Theodore Tso wrote: On Tue, May 22, 2007 at 06:11:27PM +0200, Jan Kara wrote: while fixing some problems with preallocation in UDF, I had a look how ext2 solves similar problems. I found out that ext2_discard_prealloc() is called on every iput() from ext2_put_inode(). Is it really appropriate? I don't see a reason for doing so... I agree, it's probably not appropriate. It's been that way for a long time, though (since 2.4.20). It's not as horrible as it seems since unlike traditional Unix systems, we don't call iput() as often, since for example operations like close() end up calling dput(), which decrements the ref. count on dentry, not the inode. But it would probably be better to check to see if i_count is 1 before deciding to discard the preallocation. OK, but then you could move the code to drop_inode() which is called at exactly that moment... I've been thinking more about it when fixing UDF. Discarding prealloc at drop_inode() has the disadvantage that symlinks/directories will keep their preallocated blocks until inodes are evicted from memory. Which is probably why ext2 discards prealloc on iput(). Also I found slightly misleading the comment at ext2_release_file(). As far as I understand the code it isn't when /all/ files are closed but rather when all fd's for given filp are closed. I.e. if you open the same file two times, -release will get called once for each open. Am I right? Yep! If so, then also calling ext2_discard_prealloc() from ext2_release_file() is suboptimal, isn't it? Yes, although it's a bit better because only discaord the preallocation if the file descriptor was opened for writing. The file could be opened for writing by multiple file descriptors, true, but in that case it's likely that the write pattern will be a random access one anyway, so the preallocated region is less useful. OK, but still we could use e.g. i_writecount to check that we drop the last descriptor for writing... P.S. Note that ext3 and ext4 use a different preallocation scheme; Yes, I know that. That's why I looked at ext2 when searching for inspiration how to fix UDF :) still patches to fix the comments might not be a bad idea, since it might save confusion by others later on. Ok, will write one. Honza -- Jan Kara [EMAIL PROTECTED] SuSE CR Labs - To unsubscribe from this list: send the line unsubscribe linux-ext4 in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: ext2_discard_prealloc() called on each iput?
On Tue, May 22, 2007 at 06:11:27PM +0200, Jan Kara wrote: while fixing some problems with preallocation in UDF, I had a look how ext2 solves similar problems. I found out that ext2_discard_prealloc() is called on every iput() from ext2_put_inode(). Is it really appropriate? I don't see a reason for doing so... I agree, it's probably not appropriate. It's been that way for a long time, though (since 2.4.20). It's not as horrible as it seems since unlike traditional Unix systems, we don't call iput() as often, since for example operations like close() end up calling dput(), which decrements the ref. count on dentry, not the inode. But it would probably be better to check to see if i_count is 1 before deciding to discard the preallocation. Also I found slightly misleading the comment at ext2_release_file(). As far as I understand the code it isn't when /all/ files are closed but rather when all fd's for given filp are closed. I.e. if you open the same file two times, -release will get called once for each open. Am I right? Yep! If so, then also calling ext2_discard_prealloc() from ext2_release_file() is suboptimal, isn't it? Yes, although it's a bit better because only discaord the preallocation if the file descriptor was opened for writing. The file could be opened for writing by multiple file descriptors, true, but in that case it's likely that the write pattern will be a random access one anyway, so the preallocated region is less useful. Regards, - Ted P.S. Note that ext3 and ext4 use a different preallocation scheme; still patches to fix the comments might not be a bad idea, since it might save confusion by others later on. - To unsubscribe from this list: send the line unsubscribe linux-ext4 in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
