Re: fishy ->put_inode usage in ntfs

[Christoph Hellwig <hch@lst.de>]

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?