Re: [UNIONFS] 00/29 Unionfs and related patches pre-merge review (v2)

[Al Viro <viro@ZenIV.linux.org.uk>]

On Sat, Jan 26, 2008 at 12:08:30AM -0500, Erez Zadok wrote:

> > 	* lock_parent(): who said that you won't get dentry moved
> > before managing to grab i_mutex on parent?  While we are at it,
> > who said that you won't get dentry moved between fetching d_parent
> > and doing dget()?  In that case parent could've been _freed_ before
> > you get to dget().
> 
> OK, so looks like I should use dget_parent() in my lock_parent(), as I've
> done elsewhere.  I'll also take a look at all instances in which I get
> dentry->d_parent and see if a d_lock is needed there.
 
dget_parent() doesn't deal with the problem of rename() done directly
in that layer while you'd been waiting for i_mutex.

> > +       lock_rename(lower_old_dir_dentry, lower_new_dir_dentry);
> > +       err = vfs_rename(lower_old_dir_dentry->d_inode, lower_old_dentry,
> > +                        lower_new_dir_dentry->d_inode, lower_new_dentry);
> > +       unlock_rename(lower_old_dir_dentry, lower_new_dir_dentry);
> > 
> > Uh-huh...  To start with, what guarantees that your lower_old_dentry
> > is still a child of your lower_old_dir_dentry?
> 
> We dget/dget_parent the old/new dentry and parents a few lines above
> (actually, it looked like I forgot to dget(lower_new_dentry) -- fixed).

And?  Having a reference to dentry does not prevent it being moved
elsewhere by direct rename(2) in that layer.  It will exist, that
much is guaranteed by grabbing a reference.  However, there is no
warranties whatsoever that by the time you get i_mutex on what had
once been its parent, it will still remain the parent of our dentry.

> BTW, my sense of the relationship b/t upper and lower objects and their
> validity in a stackable f/s, is that it's similar to the relationship b/t
> the NFS client and server -- the client can't be sure that a file on the
> server doesn't change b/t ->revalidate and ->op (hence nfs's reliance on dir
> mtime checks).

You are thinking about non-interesting case.  _Files_ are not much
of a problem.  Directory tree is.  The real problems with all unionfs and
stacking implementations I've seen so far, all way back to Heidemann et.al.
start when topology of the underlying layer changes.  If you have clear
semantics for unionfs behaviour in presence of such things, by all means,
publish it - as far as I know *nobody* had done that; not even on the
"what should we see when..." level, nevermind the implementation.
 
> Perhaps this general topic is a good one to discuss at more length at LSF?
> Suggestions are welcome.

It would; I honestly do not know if the problem is solvable with the
(lack of) constraints you apparently want.  Again, the real PITA begins
when you start dealing with pieces of underlying trees getting moved
around, changing parents, etc.  Cross-directory rename() certainly rates
very high on the list of "WTF had they been smoking in UCB?" misfeatures,
but it's there and it has to be dealt with.

BTW, and that's a completely unrelated story, I'd rather see whiteouts
done directly by filesystems involved - it would simplify the life big
way.  How about adding a dir->i_op->whiteout(dir, dentry) and seeing if
your variant could be turned into such a method to be used by really
piss-poor filesystems?  All UFS-related ones (including ext*) can trivially
support whiteouts without any PITA; adding them to tmpfs is also not a big
deal and anything that caches inode type in directory entries should be
easy to extend in the same way as ext*/ufs...