Re: fishy ->put_inode usage in ntfs

[Anton Altaparmakov <aia21@cam.ac.uk>]

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/