Re: fatfs write support

[Marco Gerards]

tb@becket.net (Thomas Bushnell, BSG) writes:

> Marco Gerards <metgerards@student.han.nl> writes:
>
>> It already is the same lock, that is causing the problem.  We don't
>> know if it is locked or not when write_node is called.  I don't see
>> any change here.
>
> Huh?  In diskfs, every node has one lock.  That lock is not the same
> as the lock on the directory holding the file.
>
> In fatfs, to write an inode, you must modify the directory it is in,
> which requires locking the directory.  And, you of course hold the
> inode locked which you are writing.  This causes deadlock problems,
> because you can't try to lock a directory while you are holding a lock
> on a file within the directory.
>
> I'm suggesting (and it's just a guess; I don't claim to have the
> details worked out) that you have *only* a lock for the directory, and
> *no* lock for a regular inode, at all.
>
> That is, don't use <node>->lock at all.  This may require more
> substantial changes to diskfs than my first message intimated, on
> second look.  But this is what I thought ten years ago would probably
> be the right way to implement fatfs (though I freely admit that I
> didn't attempt to think through all the details, and I might well be
> wrong).
>
> One problem with this strategy is that it is not at all clear what to
> do when you are treating directories.  Which all means that perhaps
> this is indeed wrong.
>
>> Do you mean that we have to split up the lock and make it more refined?
>
> Ok, just thinking aloud: there are *two* distinct things that we are
> conflating in the one <node>->lock that currently exists in diskfs:
>
> First, <node>->lock governs modifications to the file itself that is
> represented by <node>.
>
> Second, <node>->lock governs modifications to the metadata for the
> file.
>
> Now the reason for conflating these obviously includes that some of
> the metadata is directly connected to modifying the file (especially
> the file size).  But we have trouble now, that the metadata for a fatfs
> inode is found inside some other file.
>
> I can think of two strategies that might work here (I admit I still
> haven't wrapped my head around the description of the strategy you
> suggested in your email):
>
> 1) Change diskfs to have a special "fatfs" option (perhaps called
>    something like "diskfs_metadata_in_directories" or whatnot).  If
>    the fatfs flag is set, then the directory must always be locked
>    when diskfs_write_disknode is called.  This would require very
>    careful work throughout diskfs.  When the nodes were just looked
>    up, this requires only preserving the locks on the directories
>    longer.  In other cases (say, file_chown), if the fatfs flag is set
>    then diskfs would have to acquire the directory lock before locking
>    the file, and then unlock both after the nput call is complete.
>
>    I believe this is the right strategy.  It does involve more pain,
>    and risks the ire of lots of other people if diskfs gets mangled in
>    the process.  But I do think this is the Right Thing.

The main problem I am seeing here is that the caller of
diskfs_write_disknode should lookup the directory.  For file_chown for
example this would be hard to do, especially from within libdiskfs
because it does not know how to lookup the directory the node is in.

However, this is possible from within fatfs because fatfs has more
specific knowledge.

> 2) You could rig up a special "directory writing unit" that would live
>    entirely inside fatfs, which would have its own separate interlocks
>    for directory writing.  The idea is that you would just reach
>    inside the directory and rewrite it willy nilly without having
>    locked the directory.  That's safe as long as you interact
>    correctly with direnter, lookup, directory deletion, and everything
>    else.  It might well be *more* pain that (1) above.  I do not think
>    merely adding a reader/writer lock will do the trick.  This is, I
>    think, something like the plan in your first email on the subject,
>    but as I haven't studied it carefully yet, I can't be sure.

This is what I proposed.  Inside diskfs_lookup_hard,
diskfs_dirrewrite_hard, etc. you just have to add that rwlock so it
protects access to the directory entry.  I will try to implement this
tomorrow (it is not that hard to do) and will look how it turns out.

Thanks,
Marco