fatfs locking

[Marcus Brinkmann]

Hi,

Thomas, I am CC you directly because it is your veto to overcome, and
I am not sure how often you check hurd-devel these days.

As far as I understand Marco, this is the showstopper in write support
for fatfs, which he really, really wants to finish.  So, please let's
pull together and take another look at this issue.

I am quoting all relevant discussion below.  Frankly, I am with Marco
and Roland here, in that what Roland proposes sounds like a sane
solution.  I don't understand Thomas' criticism.  Whatever problems
there might be with the order of locking at lookup has little to do
with the issue at hand.  The change proposed does not change the order
of lookup.  If .. needs special treatment, it can get this special
treatment at the diskfs level or the filesystem level.  If the
.. treatment is the only thing under contention, then we can leave it
out of the proposed change and only add the argument we want to add,
so fatfs can do the right thing.

The only reason to not apply the patch would be if we had to break
other filesystems, or change their behaviour in a way we don't want,
or if we couldn't make use of this change in fatfs to enable write
support.  First, let's consider the case without touching the .. case:
In this case, adding an argument to the interface can not break
existing filesystems (they can just ignore the new argument).  Will it
allow to fix fatfs?  This, too, appears to be the case.

If we can agree on this, then the important part of the patch can go
in, and I am happy to leave the minor part of the patch, the lookup of
"..", between Roland and Thomas if they want to carry it out.

So, unless Roland changed his mind on this, there are only two
questions to ask, and they both go to Thomas:

* Do you agree that it makes sense to add an argument to the
  interfaces as Roland describes it?

* If you do not agree, how can we make you to agree? :)

The bonus question is if you also agree to do away with the .. special
case in name cache, leaving it to the filesystems to deal with it.
However, the answer to the above two questions should not depend on
the answer to the bonus question.

Thanks,
Marcus

From: address@hidden

> Currently I'm working on fatfs and I ran into a little problem:
>
> FAT doesn't have inodes, so fatfs has to lock the node of the
> directory that contains the node for which diskfs_cached_lookup is
> called. diskfs_cached_lookup is called by diskfs, before that call the
> directory node is locked or unlocked by diskfs and fatfs doesn't know if
> diskfs locked the node or if it didn't. Because of this fatfs doesn't
> know if it should lock the directory node.

From: Roland McGrath <address@hidden>

> It seems reasonable to me to leave it up to the filesystem-specific code to
> decide what nodes it might need to lock, and just give it enough
> information to avoid deadlock.  I have in my tree a slightly different
> change that adds a struct node * argument to diskfs_cached_lookup instead
> of a flag, indicating the directory node (or none if null) that the caller
> has locked.  I think that by using this the special case for ".." in
> libdiskfs/name-cache.c can be removed.  I have changes that add the
> argument, remove the ".." special case for unlocking in
> diskfs_check_lookup_cache, and instead makes each diskfs_cached_lookup
> implementation check for the lookup matching the already-locked node.
> 
> It occurs to me that without this change, if a directory in a ufs or ext2fs
> filesystem contains a link to itself by a name other than "..", then a
> lookup of that name will deadlock the directory node.  (That is probably an
> invalid state that fsck would fix, but still.)  Am I right about that?
> 
> I'll put these changes in if Thomas agrees it makes sense.

From: address@hidden (Thomas Bushnell, BSG)

> It is assumed that (with the exception of "..", and links to self) the
> structure of links is a DAG.
> 
> The rule that you must hold a lock on the directory to acquire the
> node in the directory is very old in Unix, and works around the
> following situation:
> 
> lock dir
> look up name, record inode number N
> unlock dir
> 
>   other thread now unlinks inode N, which then gets allocated as some
>   new node
> 
> lock inode N
> 
> whoops--wrong node!
> 
> (There is also a problem if the node is freed but not allocated; this
> problem is more easily detectable and can be worked around.)
> 
> The ".." case works by:
> 
> lock subdir
> look up .., record parent inode number N
> unlock subdir -- holding a reference to prevent reallocation
> lock parent inode N
> lock subdir
> look up ".." again, make sure it still points at N
> unlock subdir

And in another reply:

> Ok, I've now had a chance to think more about this.
> 
> This is basically sure to be wrong, at least, it will suffer from the
> consistency problems that I mentioned in the previous message I sent.
> 
> I will think about how to make fatfs work, but I don't think this kind
> of "solution" is at all right.