ext2_discard_prealloc() called on each iput?

ext2_discard_prealloc() called on each iput?

[Jan Kara]

  Hello,

  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...
  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?
If so, then also calling ext2_discard_prealloc() from ext2_release_file()
is suboptimal, isn't it?
					Thanks for answer
								Honza
-- 
Jan Kara <jack@suse.cz>
SuSE CR Labs

Re: ext2_discard_prealloc() called on each iput?

[Theodore Tso]

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.

Re: ext2_discard_prealloc() called on each iput?

[Jan Kara]

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 <jack@suse.cz>
SuSE CR Labs

Re: ext2_discard_prealloc() called on each iput?

[Theodore Tso]

On Mon, May 28, 2007 at 06:04:20PM +0200, Jan Kara wrote:
>   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().

We should never be preallocating for symlinks (it just doesn't make
any sense), and for directories, it usually doesn't make sense,
either.  I don't recall that ext2 did preallocation for
symlinks/directories; I seem to recall it was only for normal files.
If it isn't, I'd argue that's a bug that should be fixed.

In either case, discarding on iput() doesn't help since we don't hold
a reference count on the inode, but rather the dentry .

>   OK, but still we could use e.g. i_writecount to check that we drop the
> last descriptor for writing...

That would be better, yes.

						- Ted

Re: ext2_discard_prealloc() called on each iput?

[Mingming Cao]

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

Re: ext2_discard_prealloc() called on each iput?

[Jan Kara]

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 <jack@suse.cz>
SuSE CR Labs

Re: ext2_discard_prealloc() called on each iput?

[Jan Kara]

[-- Attachment #1: Type: text/plain, Size: 638 bytes --]

On Wed 23-05-07 08:37:43, Theodore Tso wrote:
> >   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?
  <snip>
> still patches to fix the comments might not be a bad idea, since it
> might save confusion by others later on.
  OK, attached is a patch that fixes the comment. Andrew, please put it
into your patch queue... Thanks.

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

[-- Attachment #2: ext2-2.6.21-1-comment_fix.diff --]
[-- Type: text/x-patch, Size: 881 bytes --]

Fix a comment when ext2_release_file() is called.

Signed-off-by: Jan Kara <jack@suse.cz>

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.000000000 +0200
+++ linux-2.6.21-1-ext2_comment_fix/fs/ext2/file.c	2007-05-29 12:15:59.000000000 +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)
 {

Re: ext2_discard_prealloc() called on each iput?

[Theodore Tso]

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 <jack@suse.cz>

Acked-by: "Theodore Ts'o" <tytso@mit.edu>

> 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.000000000 +0200
> +++ linux-2.6.21-1-ext2_comment_fix/fs/ext2/file.c	2007-05-29 12:15:59.000000000 +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)
>  {