Why do we pass in a directory and a dentry to lookup() and rename()

Why do we pass in a directory and a dentry to lookup() and rename()

[Theodore Ts'o]

In the inode operations functions lookup() and rename():

	struct dentry * (*lookup)(struct inode *inode,
	       	      		  struct dentry *dentry,
				  struct nameidata *nd);

	int (*rename)(struct inode *old_dir, struct dentry *old_dentry,
	    	      struct inode *new_dir, struct dentry *new_entry);

... is the fact that we pass in the inode superfluous?

i.e., it looks like in all cases one can obtain the inode by looking at
dentry->d_parent.  Or am I missing something?

Thanks,

							- Ted

Re: Why do we pass in a directory and a dentry to lookup() and rename()

[Ted Ts'o]

On Mon, Dec 27, 2010 at 11:14:05AM +0530, Rajat Sharma wrote:
> Hi Theodore,
> 
> In both the operations, inodes refer to parent directory while dentry refers
> to child. Hope this clarifies.

Sure, but we can get to the parent directory's inode from the dentry via:

      dentry->d_parent->d_inode

... can we not?

					- Ted

Re: Why do we pass in a directory and a dentry to lookup() and rename()

[Neil Brown]

On Sun, 26 Dec 2010 16:45:51 -0500 "Theodore Ts'o" <tytso@mit.edu> wrote:

> In the inode operations functions lookup() and rename():
> 
> 	struct dentry * (*lookup)(struct inode *inode,
> 	       	      		  struct dentry *dentry,
> 				  struct nameidata *nd);
> 
> 	int (*rename)(struct inode *old_dir, struct dentry *old_dentry,
> 	    	      struct inode *new_dir, struct dentry *new_entry);
> 
> ... is the fact that we pass in the inode superfluous?
> 
> i.e., it looks like in all cases one can obtain the inode by looking at
> dentry->d_parent.  Or am I missing something?
> 

I'd guess that it is "historical reasons".

Some times it isn't safe to de-reference ->d_parent without extra locking
(via dget_parent) so there could be cases where passing an
already-ref-counted pointer might be cleaner, but during lookup and rename
there must be sufficient locking so that ->d_parent cannot change.
So all you could gain by passing the pointer separately is avoiding a single
memory reference.  As d_parent is almost certainly in cache when these are
called, that would not be a big gain.

It might be worth checking that the code doesn't get bigger if you drop the
arg and use ->d_parent instead, but assuming it doesn't (which seems likely),
I would agree that passing the parent inode explicitly is unnecessary.

NeilBrown

Re: Why do we pass in a directory and a dentry to lookup() and rename()

[Al Viro]

On Tue, Dec 28, 2010 at 08:46:19PM +1100, Neil Brown wrote:

> Some times it isn't safe to de-reference ->d_parent without extra locking
> (via dget_parent) so there could be cases where passing an
> already-ref-counted pointer might be cleaner, but during lookup and rename
> there must be sufficient locking so that ->d_parent cannot change.
> So all you could gain by passing the pointer separately is avoiding a single
> memory reference.  As d_parent is almost certainly in cache when these are
> called, that would not be a big gain.
> 
> It might be worth checking that the code doesn't get bigger if you drop the
> arg and use ->d_parent instead, but assuming it doesn't (which seems likely),
> I would agree that passing the parent inode explicitly is unnecessary.

In fact, all directory operations have relevant ->d_parent stable; a lot
of instances rely on that, actually.  So locking isn't a problem.

We could switch, but I'm not sure if it's worth the code churn.  Do we
really win anything there?  On targets where everything is pushed on
stack - probably yes, but I suspect that for majority of those methods
it'll be a loss on just about every target.  _Maybe_ it's not true for
->rename() (more arguments than for everything else), but...

Hell knows; I suspect that it might be worth playing with if/when we go
for explicit passing of creds to the methods.  Extra argument plus the
need to go through the instances anyway might make it worth bothering.