fishy ->put_inode usage in ntfs

fishy ->put_inode usage in ntfs

[Christoph Hellwig]

Hi Anton,

the inode->i_count useage in ntfs_put_inode is possibly ract vecause we
could get anoher reference while you're looking at it.  For the index
inode case moving this to clear_inode (which is called after we released
the last reference) should be fine, but I don't understand the case for
directories - why are you checking for i_count beeing 2 there and not
one?

Re: fishy ->put_inode usage in ntfs

[Anton Altaparmakov]

Hi Christoph,

On Thu, 2004-10-14 at 12:26, Christoph Hellwig wrote:
> the inode->i_count useage in ntfs_put_inode is possibly ract vecause we
> could get anoher reference while you're looking at it.

Hm.  For the directory inode case I don't think this is a problem
because all places that use NTFS_I(vfs inode)->itype.index.bmp_ino hold
i_sem of the vfs inode which guarantees that at least someone is holding
a reference on the vfs inode which in turn guarantees that the i_count
cannot drop to levels that would affect ntfs_put_inode().

Explanation: The person who holds i_sem also has 1 i_count and if
bmp_ino is set this also has 1 i_count on its parent inode thus we now
have a minimum i_count of 2.  To get into ntfs_put_inode() at this point
in time someone would have to do iput() and they better have 1 i_count
before they do the iput() so we now have a minimum i_count of 3 and no
code in ntfs_put_inode() would trigger.

Only when the last external user of the inode does iput(),
ntfs_put_inode() is called with i_count of 2.  ntfs_put_inode() then
iput()s the bmp_ino attribute inode.

This in turn does iput() on the parent inode which drops i_count once
more and now clear_inode() will be called by the VFS.

We cannot move this code to clear_inode() as it would never get called
since the i_count would never drop to 0 due to the existence of the
bmp_ino attribute inode which in turn will never have its i_count drop
to zero due to it being attached to the parent inode.  Catch 22.

If someone does get a new reference while we are running the above this
is also fine as all users of NTFS_I(vfs inode)->itype.index.bmp_ino
check bmp_ino for being NULL and if it is they do an ntfs_attr_iget()
and attach it on NTFS_I(vfs inode)->itype.index.bmp_ino again.

The reason for doing this is to not to have to ntfs_attr_iget() the
bmp_ino every time we need to use it (e.g. every ->readdir()
invocation).  So it may look odd but it is a speedup worth doing IMO.

Hm, I can now see that there is a small race window here and this is
simply fixed by doing the setting to NULL of bmp_ino before doing the
iput() of bmp_ino.  Thanks for pointing this problem area out to!  (-:

> For the index inode case moving this to clear_inode (which is called
> after we released the last reference) should be fine,

Indeed.  I will move it.  Thanks!  (-:

> but I don't understand the case for directories - why are you checking
> for i_count beeing 2 there and not one?

I hope I have explained this above.  If I failed please do ask again...

Best regards,

	Anton
-- 
Anton Altaparmakov <aia21 at cam.ac.uk> (replace at with @)
Unix Support, Computing Service, University of Cambridge, CB2 3QH, UK
Linux NTFS maintainer / IRC: #ntfs on irc.freenode.net
WWW: http://linux-ntfs.sf.net/, http://www-stu.christs.cam.ac.uk/~aia21/

Re: fishy ->put_inode usage in ntfs

[Matthew Wilcox]

On Thu, Oct 14, 2004 at 01:39:30PM +0100, Anton Altaparmakov wrote:
> Hm, I can now see that there is a small race window here and this is
> simply fixed by doing the setting to NULL of bmp_ino before doing the
> iput() of bmp_ino.  Thanks for pointing this problem area out to!  (-:

If you're going to rely on ordering like this, you must at least use
wmb() to ensure that neither the compiler nor the processor reorders
your stores.

-- 
"Next the statesmen will invent cheap lies, putting the blame upon 
the nation that is attacked, and every man will be glad of those
conscience-soothing falsities, and will diligently study them, and refuse
to examine any refutations of them; and thus he will by and by convince 
himself that the war is just, and will thank God for the better sleep 
he enjoys after this process of grotesque self-deception." -- Mark Twain

Re: fishy ->put_inode usage in ntfs

[Christoph Hellwig]

On Thu, Oct 14, 2004 at 01:39:30PM +0100, Anton Altaparmakov wrote:
> Hi Christoph,
> 
> On Thu, 2004-10-14 at 12:26, Christoph Hellwig wrote:
> > the inode->i_count useage in ntfs_put_inode is possibly ract vecause we
> > could get anoher reference while you're looking at it.
> 
> Hm.  For the directory inode case I don't think this is a problem
> because all places that use NTFS_I(vfs inode)->itype.index.bmp_ino hold
> i_sem of the vfs inode which guarantees that at least someone is holding
> a reference on the vfs inode which in turn guarantees that the i_count
> cannot drop to levels that would affect ntfs_put_inode().
> 
> Explanation: The person who holds i_sem also has 1 i_count and if
> bmp_ino is set this also has 1 i_count on its parent inode thus we now
> have a minimum i_count of 2.  To get into ntfs_put_inode() at this point
> in time someone would have to do iput() and they better have 1 i_count
> before they do the iput() so we now have a minimum i_count of 3 and no
> code in ntfs_put_inode() would trigger.
> 
> Only when the last external user of the inode does iput(),
> ntfs_put_inode() is called with i_count of 2.  ntfs_put_inode() then
> iput()s the bmp_ino attribute inode.
> 
> This in turn does iput() on the parent inode which drops i_count once
> more and now clear_inode() will be called by the VFS.
> 
> We cannot move this code to clear_inode() as it would never get called
> since the i_count would never drop to 0 due to the existence of the
> bmp_ino attribute inode which in turn will never have its i_count drop
> to zero due to it being attached to the parent inode.  Catch 22.
> 
> If someone does get a new reference while we are running the above this
> is also fine as all users of NTFS_I(vfs inode)->itype.index.bmp_ino
> check bmp_ino for being NULL and if it is they do an ntfs_attr_iget()
> and attach it on NTFS_I(vfs inode)->itype.index.bmp_ino again.
> 
> The reason for doing this is to not to have to ntfs_attr_iget() the
> bmp_ino every time we need to use it (e.g. every ->readdir()
> invocation).  So it may look odd but it is a speedup worth doing IMO.

Have you measured the speedup?  I don't like filesystem doings things
like this in ->put_inode at all, and indeed the plan is to get rid of
->put_inode completely.  Why do you need to hold an additional reference
anyway?  What's so special about the relation of these two inodes?

Re: fishy ->put_inode usage in ntfs

[Anton Altaparmakov]

On Thu, 2004-10-14 at 13:59, Christoph Hellwig wrote:
> On Thu, Oct 14, 2004 at 01:39:30PM +0100, Anton Altaparmakov wrote:
> > Hi Christoph,
> > 
> > On Thu, 2004-10-14 at 12:26, Christoph Hellwig wrote:
> > > the inode->i_count useage in ntfs_put_inode is possibly ract vecause we
> > > could get anoher reference while you're looking at it.
> > 
> > Hm.  For the directory inode case I don't think this is a problem
> > because all places that use NTFS_I(vfs inode)->itype.index.bmp_ino hold
> > i_sem of the vfs inode which guarantees that at least someone is holding
> > a reference on the vfs inode which in turn guarantees that the i_count
> > cannot drop to levels that would affect ntfs_put_inode().
> > 
> > Explanation: The person who holds i_sem also has 1 i_count and if
> > bmp_ino is set this also has 1 i_count on its parent inode thus we now
> > have a minimum i_count of 2.  To get into ntfs_put_inode() at this point
> > in time someone would have to do iput() and they better have 1 i_count
> > before they do the iput() so we now have a minimum i_count of 3 and no
> > code in ntfs_put_inode() would trigger.
> > 
> > Only when the last external user of the inode does iput(),
> > ntfs_put_inode() is called with i_count of 2.  ntfs_put_inode() then
> > iput()s the bmp_ino attribute inode.
> > 
> > This in turn does iput() on the parent inode which drops i_count once
> > more and now clear_inode() will be called by the VFS.
> > 
> > We cannot move this code to clear_inode() as it would never get called
> > since the i_count would never drop to 0 due to the existence of the
> > bmp_ino attribute inode which in turn will never have its i_count drop
> > to zero due to it being attached to the parent inode.  Catch 22.
> > 
> > If someone does get a new reference while we are running the above this
> > is also fine as all users of NTFS_I(vfs inode)->itype.index.bmp_ino
> > check bmp_ino for being NULL and if it is they do an ntfs_attr_iget()
> > and attach it on NTFS_I(vfs inode)->itype.index.bmp_ino again.
> > 
> > The reason for doing this is to not to have to ntfs_attr_iget() the
> > bmp_ino every time we need to use it (e.g. every ->readdir()
> > invocation).  So it may look odd but it is a speedup worth doing IMO.
> 
> Have you measured the speedup?

No.  But if ntfs_attr_iget() needs to be used every time and each time
the VFS has managed to do a clear inode() then regetting the inode (not
from icache) is very expensive.

> I don't like filesystem doings things like this in ->put_inode at all,
> and indeed the plan is to get rid of ->put_inode completely.  Why do
> you need to hold an additional reference anyway?  What's so special
> about the relation of these two inodes?

The bmp_ino is a virtual inode.  It doesn't exist on disk as an inode. 
It is an NTFS attribute of the base inode.  It cannot exist without the
base inode there.  You could neither read from nor write to this inode
without its base inode being there and you couldn't even clear_inode()
this inode without the base inode being there.  The reference is
essential I am afraid.

If ->put_inode is removed then I will have to switch to using
ntfs_attr_iget() each time or I will have to attach the inode in some
other much hackier way that doesn't use the i_count and uses my ntfs
private counter instead.

Best regards,

	Anton
-- 
Anton Altaparmakov <aia21 at cam.ac.uk> (replace at with @)
Unix Support, Computing Service, University of Cambridge, CB2 3QH, UK
Linux NTFS maintainer / IRC: #ntfs on irc.freenode.net
WWW: http://linux-ntfs.sf.net/, http://www-stu.christs.cam.ac.uk/~aia21/

Re: fishy ->put_inode usage in ntfs

[Anton Altaparmakov]

On Thu, 2004-10-14 at 13:44, Matthew Wilcox wrote:
> On Thu, Oct 14, 2004 at 01:39:30PM +0100, Anton Altaparmakov wrote:
> > Hm, I can now see that there is a small race window here and this is
> > simply fixed by doing the setting to NULL of bmp_ino before doing the
> > iput() of bmp_ino.  Thanks for pointing this problem area out to!  (-:
> 
> If you're going to rely on ordering like this, you must at least use
> wmb() to ensure that neither the compiler nor the processor reorders
> your stores.

Thanks, will do.

Best regards,

	Anton
-- 
Anton Altaparmakov <aia21 at cam.ac.uk> (replace at with @)
Unix Support, Computing Service, University of Cambridge, CB2 3QH, UK
Linux NTFS maintainer / IRC: #ntfs on irc.freenode.net
WWW: http://linux-ntfs.sf.net/, http://www-stu.christs.cam.ac.uk/~aia21/

Re: fishy ->put_inode usage in ntfs

[Anton Altaparmakov]

On Thu, 2004-10-14 at 13:44, Matthew Wilcox wrote:
> On Thu, Oct 14, 2004 at 01:39:30PM +0100, Anton Altaparmakov wrote:
> > Hm, I can now see that there is a small race window here and this is
> > simply fixed by doing the setting to NULL of bmp_ino before doing the
> > iput() of bmp_ino.  Thanks for pointing this problem area out to!  (-:
> 
> If you're going to rely on ordering like this, you must at least use
> wmb() to ensure that neither the compiler nor the processor reorders
> your stores.

Am I right that if I surround the code with down/up(i_sem) this implies
a memory barrier so I do not need wmb()?  The relevant code now is:

bvi = NULL;
down(&vi->i_sem);
if (atomic_read(&vi->i_count) == 2) {
	bvi = ni->itype.index.bmp_ino;
	if (bvi)
		ni->itype.index.bmp_ino = NULL;
}
up(&vi->i_sem);
if (bvi)
	iput(bvi);

Best regards,

	Anton
-- 
Anton Altaparmakov <aia21 at cam.ac.uk> (replace at with @)
Unix Support, Computing Service, University of Cambridge, CB2 3QH, UK
Linux NTFS maintainer / IRC: #ntfs on irc.freenode.net
WWW: http://linux-ntfs.sf.net/, http://www-stu.christs.cam.ac.uk/~aia21/

Re: fishy ->put_inode usage in ntfs

[Christoph Hellwig]

On Thu, Oct 14, 2004 at 02:26:45PM +0100, Anton Altaparmakov wrote:
> > I don't like filesystem doings things like this in ->put_inode at all,
> > and indeed the plan is to get rid of ->put_inode completely.  Why do
> > you need to hold an additional reference anyway?  What's so special
> > about the relation of these two inodes?
> 
> The bmp_ino is a virtual inode.  It doesn't exist on disk as an inode. 
> It is an NTFS attribute of the base inode.  It cannot exist without the
> base inode there.  You could neither read from nor write to this inode
> without its base inode being there and you couldn't even clear_inode()
> this inode without the base inode being there.  The reference is
> essential I am afraid.
> 
> If ->put_inode is removed then I will have to switch to using
> ntfs_attr_iget() each time or I will have to attach the inode in some
> other much hackier way that doesn't use the i_count and uses my ntfs
> private counter instead.

Coming back to this issue.  Why do you need to refcount bmp_ino at all?
Can someone ever grab a reference separate from it's master inode?

Re: fishy ->put_inode usage in ntfs

[Anton Altaparmakov]

On Thu, 2005-02-10 at 11:47 +0100, Christoph Hellwig wrote:
> On Thu, Oct 14, 2004 at 02:26:45PM +0100, Anton Altaparmakov wrote:
> > > I don't like filesystem doings things like this in ->put_inode at all,
> > > and indeed the plan is to get rid of ->put_inode completely.  Why do
> > > you need to hold an additional reference anyway?  What's so special
> > > about the relation of these two inodes?
> > 
> > The bmp_ino is a virtual inode.  It doesn't exist on disk as an inode. 
> > It is an NTFS attribute of the base inode.  It cannot exist without the
> > base inode there.  You could neither read from nor write to this inode
> > without its base inode being there and you couldn't even clear_inode()
> > this inode without the base inode being there.  The reference is
> > essential I am afraid.
> > 
> > If ->put_inode is removed then I will have to switch to using
> > ntfs_attr_iget() each time or I will have to attach the inode in some
> > other much hackier way that doesn't use the i_count and uses my ntfs
> > private counter instead.
> 
> Coming back to this issue.  Why do you need to refcount bmp_ino at all?

I am not sure what you mean.  The VFS layer does reference counting on
inodes.  I have no choice in the matter.

> Can someone ever grab a reference separate from it's master inode?

Again, not sure what you mean.  Could you elaborate?

Best regards,

        Anton
-- 
Anton Altaparmakov <aia21 at cam.ac.uk> (replace at with @)
Unix Support, Computing Service, University of Cambridge, CB2 3QH, UK
Linux NTFS maintainer / IRC: #ntfs on irc.freenode.net
WWW: http://linux-ntfs.sf.net/ & http://www-stu.christs.cam.ac.uk/~aia21/

Re: fishy ->put_inode usage in ntfs

[Christoph Hellwig]

On Thu, Feb 10, 2005 at 02:40:39PM +0000, Anton Altaparmakov wrote:
> I am not sure what you mean.  The VFS layer does reference counting on
> inodes.  I have no choice in the matter.
> 
> > Can someone ever grab a reference separate from it's master inode?
> 
> Again, not sure what you mean.  Could you elaborate?

ntfs_read_locked_attr_inode() does igrab on the 'parent' inode
currently.  What do you need this for exactly - the attr inode
goes away anyway when clear_inode is called on that 'parent' inode
(in my scheme).

Re: fishy ->put_inode usage in ntfs

[Anton Altaparmakov]

On Thu, 2005-02-10 at 15:42 +0100, Christoph Hellwig wrote:
> On Thu, Feb 10, 2005 at 02:40:39PM +0000, Anton Altaparmakov wrote:
> > I am not sure what you mean.  The VFS layer does reference counting on
> > inodes.  I have no choice in the matter.
> > 
> > > Can someone ever grab a reference separate from it's master inode?
> > 
> > Again, not sure what you mean.  Could you elaborate?
> 
> ntfs_read_locked_attr_inode() does igrab on the 'parent' inode
> currently.  What do you need this for exactly - the attr inode
> goes away anyway when clear_inode is called on that 'parent' inode
> (in my scheme).

If the igrab() were not done, it would be possible for clear_inode to be
called on the 'parent' inode whilst at the same time one or more attr
inodes (belonging to this 'parent') are in use and Bad Things(TM) would
happen...

Best regards,

        Anton
-- 
Anton Altaparmakov <aia21 at cam.ac.uk> (replace at with @)
Unix Support, Computing Service, University of Cambridge, CB2 3QH, UK
Linux NTFS maintainer / IRC: #ntfs on irc.freenode.net
WWW: http://linux-ntfs.sf.net/ & http://www-stu.christs.cam.ac.uk/~aia21/

Re: fishy ->put_inode usage in ntfs

[Anton Altaparmakov]

On Thu, 2005-02-10 at 14:48 +0000, Anton Altaparmakov wrote:
> On Thu, 2005-02-10 at 15:42 +0100, Christoph Hellwig wrote:
> > On Thu, Feb 10, 2005 at 02:40:39PM +0000, Anton Altaparmakov wrote:
> > > I am not sure what you mean.  The VFS layer does reference counting on
> > > inodes.  I have no choice in the matter.
> > > 
> > > > Can someone ever grab a reference separate from it's master inode?
> > > 
> > > Again, not sure what you mean.  Could you elaborate?
> > 
> > ntfs_read_locked_attr_inode() does igrab on the 'parent' inode
> > currently.  What do you need this for exactly - the attr inode
> > goes away anyway when clear_inode is called on that 'parent' inode
> > (in my scheme).
> 
> If the igrab() were not done, it would be possible for clear_inode to be
> called on the 'parent' inode whilst at the same time one or more attr
> inodes (belonging to this 'parent') are in use and Bad Things(TM) would
> happen...

The igrab() effectively guarantees that iput() is called on all attr
inodes before clear_inode on the 'parent' can be invoked.

Best regards,

        Anton
-- 
Anton Altaparmakov <aia21 at cam.ac.uk> (replace at with @)
Unix Support, Computing Service, University of Cambridge, CB2 3QH, UK
Linux NTFS maintainer / IRC: #ntfs on irc.freenode.net
WWW: http://linux-ntfs.sf.net/ & http://www-stu.christs.cam.ac.uk/~aia21/

Re: fishy ->put_inode usage in ntfs

[Christoph Hellwig]

On Thu, Feb 10, 2005 at 02:48:26PM +0000, Anton Altaparmakov wrote:
> If the igrab() were not done, it would be possible for clear_inode to be
> called on the 'parent' inode whilst at the same time one or more attr
> inodes (belonging to this 'parent') are in use and Bad Things(TM) would
> happen...

What bad thing specificly?  If there's shared information we should
probably refcount them separately.

Re: fishy ->put_inode usage in ntfs

[Christoph Hellwig]

On Thu, Feb 10, 2005 at 02:50:02PM +0000, Anton Altaparmakov wrote:
> > If the igrab() were not done, it would be possible for clear_inode to be
> > called on the 'parent' inode whilst at the same time one or more attr
> > inodes (belonging to this 'parent') are in use and Bad Things(TM) would
> > happen...
> 
> The igrab() effectively guarantees that iput() is called on all attr
> inodes before clear_inode on the 'parent' can be invoked.

Yes, but why exactly is this important.  It looks like you're absuing
the refcount on the 'parent' inode for some shared data?

Re: fishy ->put_inode usage in ntfs

[Anton Altaparmakov]

On Thu, 2005-02-10 at 15:50 +0100, Christoph Hellwig wrote:
> On Thu, Feb 10, 2005 at 02:48:26PM +0000, Anton Altaparmakov wrote:
> > If the igrab() were not done, it would be possible for clear_inode to be
> > called on the 'parent' inode whilst at the same time one or more attr
> > inodes (belonging to this 'parent') are in use and Bad Things(TM) would
> > happen...
> 
> What bad thing specificly?  If there's shared information we should
> probably refcount them separately.

Each attr inode stores a pointer to its parent inode in NTFS_I(struct
inode *vi)->ext.base_ntfs_ino.  This pointer would point to random
memory if clear_inode is called on the parent whilst the attr inode is
still in use.

Best regards,

        Anton
-- 
Anton Altaparmakov <aia21 at cam.ac.uk> (replace at with @)
Unix Support, Computing Service, University of Cambridge, CB2 3QH, UK
Linux NTFS maintainer / IRC: #ntfs on irc.freenode.net
WWW: http://linux-ntfs.sf.net/ & http://www-stu.christs.cam.ac.uk/~aia21/

Re: fishy ->put_inode usage in ntfs

[Christoph Hellwig]

On Thu, Feb 10, 2005 at 02:59:32PM +0000, Anton Altaparmakov wrote:
> On Thu, 2005-02-10 at 15:50 +0100, Christoph Hellwig wrote:
> > On Thu, Feb 10, 2005 at 02:48:26PM +0000, Anton Altaparmakov wrote:
> > > If the igrab() were not done, it would be possible for clear_inode to be
> > > called on the 'parent' inode whilst at the same time one or more attr
> > > inodes (belonging to this 'parent') are in use and Bad Things(TM) would
> > > happen...
> > 
> > What bad thing specificly?  If there's shared information we should
> > probably refcount them separately.
> 
> Each attr inode stores a pointer to its parent inode in NTFS_I(struct
> inode *vi)->ext.base_ntfs_ino.  This pointer would point to random
> memory if clear_inode is called on the parent whilst the attr inode is
> still in use.

Ok.  Al Viro suggested to put a spinlock around the bmp_ino pointer and
then igrab the master inode when accessing it, iput it when done.

Below is a rather half-backed patch to implement the suggestion.  It
compiles, but I'm pretty sure I introduced various bugs.

Also the new spinlock isn't nice, it'd probably be better to reuse some
other lock for this single field.


--- 1.85/fs/ntfs/dir.c	2004-11-05 13:48:35 +01:00
+++ edited/fs/ntfs/dir.c	2005-02-13 17:34:51 +01:00
@@ -1102,7 +1102,7 @@ static int ntfs_readdir(struct file *fil
 {
 	s64 ia_pos, ia_start, prev_ia_pos, bmp_pos;
 	loff_t fpos;
-	struct inode *bmp_vi, *vdir = filp->f_dentry->d_inode;
+	struct inode *bmp_vi = NULL, *vdir = filp->f_dentry->d_inode;
 	struct super_block *sb = vdir->i_sb;
 	ntfs_inode *ndir = NTFS_I(vdir);
 	ntfs_volume *vol = NTFS_SB(sb);
@@ -1250,17 +1250,14 @@ skip_index_root:
 	/* Get the offset into the index allocation attribute. */
 	ia_pos = (s64)fpos - vol->mft_record_size;
 	ia_mapping = vdir->i_mapping;
-	bmp_vi = ndir->itype.index.bmp_ino;
+
+	bmp_vi = ntfs_grab_bmp_inode(vdir);
 	if (unlikely(!bmp_vi)) {
-		ntfs_debug("Inode 0x%lx, regetting index bitmap.", vdir->i_ino);
-		bmp_vi = ntfs_attr_iget(vdir, AT_BITMAP, I30, 4);
-		if (IS_ERR(bmp_vi)) {
-			ntfs_error(sb, "Failed to get bitmap attribute.");
-			err = PTR_ERR(bmp_vi);
-			goto err_out;
-		}
-		ndir->itype.index.bmp_ino = bmp_vi;
+		ntfs_error(sb, "Failed to get bitmap attribute.");
+		err = -EINVAL;
+		goto err_out;
 	}
+
 	bmp_mapping = bmp_vi->i_mapping;
 	/* Get the starting bitmap bit position and sanity check it. */
 	bmp_pos = ia_pos >> ndir->itype.index.block_size_bits;
@@ -1445,6 +1442,8 @@ EOD:
 abort:
 	kfree(name);
 done:
+	if (bmp_vi)
+		ntfs_ungrab_bmp_inode(vdir);
 #ifdef DEBUG
 	if (!rc)
 		ntfs_debug("EOD, fpos 0x%llx, returning 0.", fpos);
@@ -1455,6 +1454,8 @@ done:
 	filp->f_pos = fpos;
 	return 0;
 err_out:
+	if (bmp_vi)
+		ntfs_ungrab_bmp_inode(vdir);
 	if (bmp_page)
 		ntfs_unmap_page(bmp_page);
 	if (ia_page) {
@@ -1537,8 +1538,16 @@ static int ntfs_dir_fsync(struct file *f
 
 	ntfs_debug("Entering for inode 0x%lx.", vi->i_ino);
 	BUG_ON(!S_ISDIR(vi->i_mode));
-	if (NInoIndexAllocPresent(ni) && ni->itype.index.bmp_ino)
-		write_inode_now(ni->itype.index.bmp_ino, !datasync);
+
+	if (NInoIndexAllocPresent(ni)) {
+		struct inode *bmp_inode = ntfs_grab_bmp_inode(vi);
+
+		if (bmp_inode) {
+			write_inode_now(bmp_inode, !datasync);
+			ntfs_ungrab_bmp_inode(vi);
+		}
+	}
+
 	ret = ntfs_write_inode(vi, 1);
 	write_inode_now(vi, !datasync);
 	err = sync_blockdev(vi->i_sb->s_bdev);
--- 1.154/fs/ntfs/inode.c	2004-11-09 11:42:05 +01:00
+++ edited/fs/ntfs/inode.c	2005-02-13 17:40:55 +01:00
@@ -388,6 +388,7 @@ void __ntfs_init_inode(struct super_bloc
 	ni->attr_list = NULL;
 	ntfs_init_runlist(&ni->attr_list_rl);
 	ni->itype.index.bmp_ino = NULL;
+	spin_lock_init(&ni->itype.index.bmp_ino_lock);
 	ni->itype.index.block_size = 0;
 	ni->itype.index.vcn_size = 0;
 	ni->itype.index.collation_rule = 0;
@@ -414,6 +415,29 @@ inline ntfs_inode *ntfs_new_extent_inode
 	return ni;
 }
 
+/*
+ * Grab primary inode when we're using the attr inode because they 
+ * share state.
+ */
+struct inode *ntfs_grab_bmp_inode(struct inode *inode)
+{
+	struct inode *bmp_inode;
+	ntfs_inode *ni = NTFS_I(inode);
+
+	spin_lock(&ni->itype.index.bmp_ino_lock);
+	bmp_inode = ni->itype.index.bmp_ino;
+	if (bmp_inode && !igrab(inode))
+		bmp_inode = NULL;
+	spin_unlock(&ni->itype.index.bmp_ino_lock);
+
+	return inode;
+}
+
+void ntfs_ungrab_bmp_inode(struct inode *inode)
+{
+	iput(inode);
+}
+
 /**
  * ntfs_is_extended_system_file - check if a file is in the $Extend directory
  * @ctx:	initialized attribute search context
@@ -1365,7 +1389,6 @@ static int ntfs_read_locked_attr_inode(s
 	 * Make sure the base inode doesn't go away and attach it to the
 	 * attribute inode.
 	 */
-	igrab(base_vi);
 	ni->ext.base_ntfs_ino = base_ni;
 	ni->nr_extents = -1;
 
@@ -1651,7 +1674,6 @@ skip_large_index_stuff:
 	 * Make sure the base inode doesn't go away and attach it to the
 	 * index inode.
 	 */
-	igrab(base_vi);
 	ni->ext.base_ntfs_ino = base_ni;
 	ni->nr_extents = -1;
 
@@ -2102,37 +2124,6 @@ err_out:
 	return -1;
 }
 
-/**
- * ntfs_put_inode - handler for when the inode reference count is decremented
- * @vi:		vfs inode
- *
- * The VFS calls ntfs_put_inode() every time the inode reference count (i_count)
- * is about to be decremented (but before the decrement itself.
- *
- * If the inode @vi is a directory with two references, one of which is being
- * dropped, we need to put the attribute inode for the directory index bitmap,
- * if it is present, otherwise the directory inode would remain pinned for
- * ever.
- */
-void ntfs_put_inode(struct inode *vi)
-{
-	if (S_ISDIR(vi->i_mode) && atomic_read(&vi->i_count) == 2) {
-		ntfs_inode *ni = NTFS_I(vi);
-		if (NInoIndexAllocPresent(ni)) {
-			struct inode *bvi = NULL;
-			down(&vi->i_sem);
-			if (atomic_read(&vi->i_count) == 2) {
-				bvi = ni->itype.index.bmp_ino;
-				if (bvi)
-					ni->itype.index.bmp_ino = NULL;
-			}
-			up(&vi->i_sem);
-			if (bvi)
-				iput(bvi);
-		}
-	}
-}
-
 static void __ntfs_clear_inode(ntfs_inode *ni)
 {
 	/* Free all alocated memory. */
@@ -2198,18 +2189,6 @@ void ntfs_clear_big_inode(struct inode *
 {
 	ntfs_inode *ni = NTFS_I(vi);
 
-	/*
-	 * If the inode @vi is an index inode we need to put the attribute
-	 * inode for the index bitmap, if it is present, otherwise the index
-	 * inode would disappear and the attribute inode for the index bitmap
-	 * would no longer be referenced from anywhere and thus it would remain
-	 * pinned for ever.
-	 */
-	if (NInoAttr(ni) && (ni->type == AT_INDEX_ALLOCATION) &&
-			NInoIndexAllocPresent(ni) && ni->itype.index.bmp_ino) {
-		iput(ni->itype.index.bmp_ino);
-		ni->itype.index.bmp_ino = NULL;
-	}
 #ifdef NTFS_RW
 	if (NInoDirty(ni)) {
 		BOOL was_bad = (is_bad_inode(vi));
@@ -2236,15 +2215,22 @@ void ntfs_clear_big_inode(struct inode *
 
 	__ntfs_clear_inode(ni);
 
+	/* Release the base inode if we are holding it. */
 	if (NInoAttr(ni)) {
-		/* Release the base inode if we are holding it. */
-		if (ni->nr_extents == -1) {
-			iput(VFS_I(ni->ext.base_ntfs_ino));
+		ntfs_inode *base_inode = ni->ext.base_ntfs_ino;
+
+		if (S_ISDIR(VFS_I(base_inode)->i_mode)) {
+			spin_lock(&base_inode->itype.index.bmp_ino_lock);
+			base_inode->itype.index.bmp_ino = NULL;
+			iput(VFS_I(base_inode));
+			spin_unlock(&base_inode->itype.index.bmp_ino_lock);
+			ni->ext.base_ntfs_ino = NULL;
+		} else if (ni->nr_extents == -1) {
 			ni->nr_extents = 0;
 			ni->ext.base_ntfs_ino = NULL;
+			iput(VFS_I(base_inode));
 		}
 	}
-	return;
 }
 
 /**
--- 1.45/fs/ntfs/inode.h	2004-10-25 11:46:30 +02:00
+++ edited/fs/ntfs/inode.h	2005-02-13 17:32:58 +01:00
@@ -101,6 +101,7 @@ struct _ntfs_inode {
 		struct { /* It is a directory, $MFT, or an index inode. */
 			struct inode *bmp_ino;	/* Attribute inode for the
 						   index $BITMAP. */
+			spinlock_t bmp_ino_lock;
 			u32 block_size;		/* Size of an index block. */
 			u32 vcn_size;		/* Size of a vcn in this
 						   index. */
@@ -275,6 +276,9 @@ extern struct inode *ntfs_attr_iget(stru
 extern struct inode *ntfs_index_iget(struct inode *base_vi, ntfschar *name,
 		u32 name_len);
 
+extern struct inode *ntfs_grab_bmp_inode(struct inode *inode);
+extern void ntfs_ungrab_bmp_inode(struct inode *inode);
+
 extern struct inode *ntfs_alloc_big_inode(struct super_block *sb);
 extern void ntfs_destroy_big_inode(struct inode *inode);
 extern void ntfs_clear_big_inode(struct inode *vi);
@@ -295,8 +299,6 @@ extern ntfs_inode *ntfs_new_extent_inode
 extern void ntfs_clear_extent_inode(ntfs_inode *ni);
 
 extern int ntfs_read_inode_mount(struct inode *vi);
-
-extern void ntfs_put_inode(struct inode *vi);
 
 extern int ntfs_show_options(struct seq_file *sf, struct vfsmount *mnt);
 
--- 1.184/fs/ntfs/super.c	2005-01-05 03:48:14 +01:00
+++ edited/fs/ntfs/super.c	2005-02-13 17:15:47 +01:00
@@ -2186,9 +2186,6 @@ static int ntfs_statfs(struct super_bloc
 static struct super_operations ntfs_sops = {
 	.alloc_inode	= ntfs_alloc_big_inode,	  /* VFS: Allocate new inode. */
 	.destroy_inode	= ntfs_destroy_big_inode, /* VFS: Deallocate inode. */
-	.put_inode	= ntfs_put_inode,	  /* VFS: Called just before
-						     the inode reference count
-						     is decreased. */
 #ifdef NTFS_RW
 	//.dirty_inode	= NULL,			/* VFS: Called from
 	//					   __mark_inode_dirty(). */

Re: fishy ->put_inode usage in ntfs

[Anton Altaparmakov]

On Sun, 13 Feb 2005, Christoph Hellwig wrote:
> On Thu, Feb 10, 2005 at 02:59:32PM +0000, Anton Altaparmakov wrote:
> > On Thu, 2005-02-10 at 15:50 +0100, Christoph Hellwig wrote:
> > > On Thu, Feb 10, 2005 at 02:48:26PM +0000, Anton Altaparmakov wrote:
> > > > If the igrab() were not done, it would be possible for clear_inode to be
> > > > called on the 'parent' inode whilst at the same time one or more attr
> > > > inodes (belonging to this 'parent') are in use and Bad Things(TM) would
> > > > happen...
> > > 
> > > What bad thing specificly?  If there's shared information we should
> > > probably refcount them separately.
> > 
> > Each attr inode stores a pointer to its parent inode in NTFS_I(struct
> > inode *vi)->ext.base_ntfs_ino.  This pointer would point to random
> > memory if clear_inode is called on the parent whilst the attr inode is
> > still in use.
> 
> Ok.  Al Viro suggested to put a spinlock around the bmp_ino pointer and
> then igrab the master inode when accessing it, iput it when done.

No lock is needed.  It wasn't needed in the past so why should it be 
needed now?

> Below is a rather half-backed patch to implement the suggestion.  It
> compiles, but I'm pretty sure I introduced various bugs.

This patch may compile but would never work.  At all.  A few reasons I can 
think of off the top of my head whilst reading the patch are inserted in 
it below.

> Also the new spinlock isn't nice, it'd probably be better to reuse some
> other lock for this single field.

As I said I don't understand what you want a lock for.

> --- 1.85/fs/ntfs/dir.c	2004-11-05 13:48:35 +01:00
> +++ edited/fs/ntfs/dir.c	2005-02-13 17:34:51 +01:00
> @@ -1102,7 +1102,7 @@ static int ntfs_readdir(struct file *fil
>  {
>  	s64 ia_pos, ia_start, prev_ia_pos, bmp_pos;
>  	loff_t fpos;
> -	struct inode *bmp_vi, *vdir = filp->f_dentry->d_inode;
> +	struct inode *bmp_vi = NULL, *vdir = filp->f_dentry->d_inode;
>  	struct super_block *sb = vdir->i_sb;
>  	ntfs_inode *ndir = NTFS_I(vdir);
>  	ntfs_volume *vol = NTFS_SB(sb);
> @@ -1250,17 +1250,14 @@ skip_index_root:
>  	/* Get the offset into the index allocation attribute. */
>  	ia_pos = (s64)fpos - vol->mft_record_size;
>  	ia_mapping = vdir->i_mapping;
> -	bmp_vi = ndir->itype.index.bmp_ino;
> +
> +	bmp_vi = ntfs_grab_bmp_inode(vdir);
>  	if (unlikely(!bmp_vi)) {
> -		ntfs_debug("Inode 0x%lx, regetting index bitmap.", vdir->i_ino);
> -		bmp_vi = ntfs_attr_iget(vdir, AT_BITMAP, I30, 4);
> -		if (IS_ERR(bmp_vi)) {
> -			ntfs_error(sb, "Failed to get bitmap attribute.");
> -			err = PTR_ERR(bmp_vi);
> -			goto err_out;
> -		}
> -		ndir->itype.index.bmp_ino = bmp_vi;
> +		ntfs_error(sb, "Failed to get bitmap attribute.");
> +		err = -EINVAL;
> +		goto err_out;

So every time we get a concurrent clear_inode() and iget() for the same 
inode what happens?  We get your "Failed to get bitmap attribute." every 
time?  Or can clear_inode only be called once the inode is removed from 
icache?  The code you are removing is getting the inode back in the case 
that it  has disappeared (which I was able to cause really easily by 
applying even moderate read-access stress on a mounted volume)...

>  	}
> +
>  	bmp_mapping = bmp_vi->i_mapping;
>  	/* Get the starting bitmap bit position and sanity check it. */
>  	bmp_pos = ia_pos >> ndir->itype.index.block_size_bits;
> @@ -1445,6 +1442,8 @@ EOD:
>  abort:
>  	kfree(name);
>  done:
> +	if (bmp_vi)
> +		ntfs_ungrab_bmp_inode(vdir);
>  #ifdef DEBUG
>  	if (!rc)
>  		ntfs_debug("EOD, fpos 0x%llx, returning 0.", fpos);
> @@ -1455,6 +1454,8 @@ done:
>  	filp->f_pos = fpos;
>  	return 0;
>  err_out:
> +	if (bmp_vi)
> +		ntfs_ungrab_bmp_inode(vdir);
>  	if (bmp_page)
>  		ntfs_unmap_page(bmp_page);
>  	if (ia_page) {
> @@ -1537,8 +1538,16 @@ static int ntfs_dir_fsync(struct file *f
>  
>  	ntfs_debug("Entering for inode 0x%lx.", vi->i_ino);
>  	BUG_ON(!S_ISDIR(vi->i_mode));
> -	if (NInoIndexAllocPresent(ni) && ni->itype.index.bmp_ino)
> -		write_inode_now(ni->itype.index.bmp_ino, !datasync);
> +
> +	if (NInoIndexAllocPresent(ni)) {
> +		struct inode *bmp_inode = ntfs_grab_bmp_inode(vi);
> +
> +		if (bmp_inode) {
> +			write_inode_now(bmp_inode, !datasync);
> +			ntfs_ungrab_bmp_inode(vi);
> +		}
> +	}
> +
>  	ret = ntfs_write_inode(vi, 1);
>  	write_inode_now(vi, !datasync);
>  	err = sync_blockdev(vi->i_sb->s_bdev);
> --- 1.154/fs/ntfs/inode.c	2004-11-09 11:42:05 +01:00
> +++ edited/fs/ntfs/inode.c	2005-02-13 17:40:55 +01:00
> @@ -388,6 +388,7 @@ void __ntfs_init_inode(struct super_bloc
>  	ni->attr_list = NULL;
>  	ntfs_init_runlist(&ni->attr_list_rl);
>  	ni->itype.index.bmp_ino = NULL;
> +	spin_lock_init(&ni->itype.index.bmp_ino_lock);
>  	ni->itype.index.block_size = 0;
>  	ni->itype.index.vcn_size = 0;
>  	ni->itype.index.collation_rule = 0;
> @@ -414,6 +415,29 @@ inline ntfs_inode *ntfs_new_extent_inode
>  	return ni;
>  }
>  
> +/*
> + * Grab primary inode when we're using the attr inode because they 
> + * share state.
> + */
> +struct inode *ntfs_grab_bmp_inode(struct inode *inode)
> +{
> +	struct inode *bmp_inode;
> +	ntfs_inode *ni = NTFS_I(inode);
> +
> +	spin_lock(&ni->itype.index.bmp_ino_lock);
> +	bmp_inode = ni->itype.index.bmp_ino;
> +	if (bmp_inode && !igrab(inode))
> +		bmp_inode = NULL;
> +	spin_unlock(&ni->itype.index.bmp_ino_lock);
> +
> +	return inode;
> +}

What about when bmp_inode is NULL?  Surely this function should be 
regetting it?  (See above comment.)

> +
> +void ntfs_ungrab_bmp_inode(struct inode *inode)
> +{
> +	iput(inode);
> +}
> +
>  /**
>   * ntfs_is_extended_system_file - check if a file is in the $Extend directory
>   * @ctx:	initialized attribute search context
> @@ -1365,7 +1389,6 @@ static int ntfs_read_locked_attr_inode(s
>  	 * Make sure the base inode doesn't go away and attach it to the
>  	 * attribute inode.
>  	 */
> -	igrab(base_vi);
>  	ni->ext.base_ntfs_ino = base_ni;
>  	ni->nr_extents = -1;
>  
> @@ -1651,7 +1674,6 @@ skip_large_index_stuff:
>  	 * Make sure the base inode doesn't go away and attach it to the
>  	 * index inode.
>  	 */
> -	igrab(base_vi);
>  	ni->ext.base_ntfs_ino = base_ni;
>  	ni->nr_extents = -1;
>  
> @@ -2102,37 +2124,6 @@ err_out:
>  	return -1;
>  }
>  
> -/**
> - * ntfs_put_inode - handler for when the inode reference count is decremented
> - * @vi:		vfs inode
> - *
> - * The VFS calls ntfs_put_inode() every time the inode reference count (i_count)
> - * is about to be decremented (but before the decrement itself.
> - *
> - * If the inode @vi is a directory with two references, one of which is being
> - * dropped, we need to put the attribute inode for the directory index bitmap,
> - * if it is present, otherwise the directory inode would remain pinned for
> - * ever.
> - */
> -void ntfs_put_inode(struct inode *vi)
> -{
> -	if (S_ISDIR(vi->i_mode) && atomic_read(&vi->i_count) == 2) {
> -		ntfs_inode *ni = NTFS_I(vi);
> -		if (NInoIndexAllocPresent(ni)) {
> -			struct inode *bvi = NULL;
> -			down(&vi->i_sem);
> -			if (atomic_read(&vi->i_count) == 2) {
> -				bvi = ni->itype.index.bmp_ino;
> -				if (bvi)
> -					ni->itype.index.bmp_ino = NULL;
> -			}
> -			up(&vi->i_sem);
> -			if (bvi)
> -				iput(bvi);

This iput() is essential.  Your patch doesn't seem to place it anywhere.  
This means that all the bmp_ino inodes in the mounted fs will never have 
their reference dropped so when you try to umount you will get "VFS: Busy 
inodes.  Self destruct in 5 seconds."...

> -		}
> -	}
> -}
> -
>  static void __ntfs_clear_inode(ntfs_inode *ni)
>  {
>  	/* Free all alocated memory. */
> @@ -2198,18 +2189,6 @@ void ntfs_clear_big_inode(struct inode *
>  {
>  	ntfs_inode *ni = NTFS_I(vi);
>  
> -	/*
> -	 * If the inode @vi is an index inode we need to put the attribute
> -	 * inode for the index bitmap, if it is present, otherwise the index
> -	 * inode would disappear and the attribute inode for the index bitmap
> -	 * would no longer be referenced from anywhere and thus it would remain
> -	 * pinned for ever.
> -	 */
> -	if (NInoAttr(ni) && (ni->type == AT_INDEX_ALLOCATION) &&
> -			NInoIndexAllocPresent(ni) && ni->itype.index.bmp_ino) {
> -		iput(ni->itype.index.bmp_ino);
> -		ni->itype.index.bmp_ino = NULL;
> -	}

You cannot remove this code or even move it to below the write commit and 
__ntfs_clear_inode call or it will break.

>  #ifdef NTFS_RW
>  	if (NInoDirty(ni)) {
>  		BOOL was_bad = (is_bad_inode(vi));
> @@ -2236,15 +2215,22 @@ void ntfs_clear_big_inode(struct inode *
>  
>  	__ntfs_clear_inode(ni);
>  
> +	/* Release the base inode if we are holding it. */
>  	if (NInoAttr(ni)) {
> -		/* Release the base inode if we are holding it. */
> -		if (ni->nr_extents == -1) {
> -			iput(VFS_I(ni->ext.base_ntfs_ino));
> +		ntfs_inode *base_inode = ni->ext.base_ntfs_ino;

You are not testing nr_extents and thus base_inode could be NULL or worse 
could be a different meaning altogether (ni->ext is a union the meaning of 
which is determined by nr_extents which must be checked before accessing 
ni->ext).

> +
> +		if (S_ISDIR(VFS_I(base_inode)->i_mode)) {
> +			spin_lock(&base_inode->itype.index.bmp_ino_lock);
> +			base_inode->itype.index.bmp_ino = NULL;
> +			iput(VFS_I(base_inode));

Why are we putting the base inode here?  I thought your patch removes the 
getting of a reference to it...

> +			spin_unlock(&base_inode->itype.index.bmp_ino_lock);
> +			ni->ext.base_ntfs_ino = NULL;
> +		} else if (ni->nr_extents == -1) {
>  			ni->nr_extents = 0;
>  			ni->ext.base_ntfs_ino = NULL;
> +			iput(VFS_I(base_inode));

Ditto.

And when the directory is being cleared?  Where is the iput() for the 
bmp_ino?  It used to be in put_inode but you get rid of it in your patch.  
Why btw?  It seems really silly to remove it considering it is there and 
it works and it is not hurting anybody...

>  		}
>  	}
> -	return;
>  }
>  
>  /**
> --- 1.45/fs/ntfs/inode.h	2004-10-25 11:46:30 +02:00
> +++ edited/fs/ntfs/inode.h	2005-02-13 17:32:58 +01:00
> @@ -101,6 +101,7 @@ struct _ntfs_inode {
>  		struct { /* It is a directory, $MFT, or an index inode. */
>  			struct inode *bmp_ino;	/* Attribute inode for the
>  						   index $BITMAP. */
> +			spinlock_t bmp_ino_lock;
>  			u32 block_size;		/* Size of an index block. */
>  			u32 vcn_size;		/* Size of a vcn in this
>  						   index. */
> @@ -275,6 +276,9 @@ extern struct inode *ntfs_attr_iget(stru
>  extern struct inode *ntfs_index_iget(struct inode *base_vi, ntfschar *name,
>  		u32 name_len);
>  
> +extern struct inode *ntfs_grab_bmp_inode(struct inode *inode);
> +extern void ntfs_ungrab_bmp_inode(struct inode *inode);
> +
>  extern struct inode *ntfs_alloc_big_inode(struct super_block *sb);
>  extern void ntfs_destroy_big_inode(struct inode *inode);
>  extern void ntfs_clear_big_inode(struct inode *vi);
> @@ -295,8 +299,6 @@ extern ntfs_inode *ntfs_new_extent_inode
>  extern void ntfs_clear_extent_inode(ntfs_inode *ni);
>  
>  extern int ntfs_read_inode_mount(struct inode *vi);
> -
> -extern void ntfs_put_inode(struct inode *vi);
>  
>  extern int ntfs_show_options(struct seq_file *sf, struct vfsmount *mnt);
>  
> --- 1.184/fs/ntfs/super.c	2005-01-05 03:48:14 +01:00
> +++ edited/fs/ntfs/super.c	2005-02-13 17:15:47 +01:00
> @@ -2186,9 +2186,6 @@ static int ntfs_statfs(struct super_bloc
>  static struct super_operations ntfs_sops = {
>  	.alloc_inode	= ntfs_alloc_big_inode,	  /* VFS: Allocate new inode. */
>  	.destroy_inode	= ntfs_destroy_big_inode, /* VFS: Deallocate inode. */
> -	.put_inode	= ntfs_put_inode,	  /* VFS: Called just before
> -						     the inode reference count
> -						     is decreased. */
>  #ifdef NTFS_RW
>  	//.dirty_inode	= NULL,			/* VFS: Called from
>  	//					   __mark_inode_dirty(). */
> 

Best regards,

	Anton
-- 
Anton Altaparmakov <aia21 at cam.ac.uk> (replace at with @)
Unix Support, Computing Service, University of Cambridge, CB2 3QH, UK
Linux NTFS maintainer / IRC: #ntfs on irc.freenode.net
WWW: http://linux-ntfs.sf.net/ & http://www-stu.christs.cam.ac.uk/~aia21/

Re: fishy ->put_inode usage in ntfs

[David Woodhouse]

On Mon, 2005-02-14 at 20:44 +0000, Anton Altaparmakov wrote:
> So every time we get a concurrent clear_inode() and iget() for the same 
> inode what happens?  We get your "Failed to get bitmap attribute." every 
> time?  Or can clear_inode only be called once the inode is removed from 
> icache? 

I thought we declared that the concurrent clear_inode() and read_inode()
were a VFS bug, and fixed it? It's even fixed in 2.4 now isn't it?

-- 
dwmw2

Re: fishy ->put_inode usage in ntfs

[Anton Altaparmakov]

On Tue, 2005-03-01 at 23:17 +0000, David Woodhouse wrote:
> On Mon, 2005-02-14 at 20:44 +0000, Anton Altaparmakov wrote:
> > So every time we get a concurrent clear_inode() and iget() for the same 
> > inode what happens?  We get your "Failed to get bitmap attribute." every 
> > time?  Or can clear_inode only be called once the inode is removed from 
> > icache? 
> 
> I thought we declared that the concurrent clear_inode() and read_inode()
> were a VFS bug, and fixed it? It's even fixed in 2.4 now isn't it?

Is it?  I must have missed this discussion.  )-:

Best regards,

        Anton
-- 
Anton Altaparmakov <aia21 at cam.ac.uk> (replace at with @)
Unix Support, Computing Service, University of Cambridge, CB2 3QH, UK
Linux NTFS maintainer / IRC: #ntfs on irc.freenode.net
WWW: http://linux-ntfs.sf.net/ & http://www-stu.christs.cam.ac.uk/~aia21/

Re: fishy ->put_inode usage in ntfs

[David Woodhouse]

On Wed, 2005-03-02 at 08:43 +0000, Anton Altaparmakov wrote:
> > I thought we declared that the concurrent clear_inode() and read_inode()
> > were a VFS bug, and fixed it? It's even fixed in 2.4 now isn't it?
> 
> Is it?  I must have missed this discussion.  )-:

Wasn't that why we backported __wait_on_freeing_inode() to 2.4?

-- 
dwmw2