lkml.org 
[lkml]   [1997]   [Aug]   [10]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: patch for fs/dcache race


On Sun, 10 Aug 1997, Bill Hawes wrote:
>
> I took a look at the new d_move code, and it does look efficient. My
> new concern though is whether there could have been references made to
> the new target dentry before the switch, which will now be invalid. The
> rename code is fraught with races, and it may be possible that the new
> dentry is already being used.

Oh, yes. The easiest way to make _sure_ ("look ma, no races") that the new
dentry is used is something like this:

foo = open("duh", O_CREAT, 0666);
bar = open("huh", O_CREAT, 0666);
rename("huh", "duh");

where the rename operation will delete "duh", but duh will still be open
because we have an open file pointer (foo) pointing to it.

What will happen is:

- d_move() will "switch" the dentries for "duh" and "huh" (resulting in
the rename we wanted, ie "huh" will now be called "duh"), and then do a
d_delete() on the old "duh" (now called "huh").
- d_delete() will work the same way it does for any dentry: it looks if
we have somebody using the entry (we do), and then unhashes it from the
parent. If we hadn't had anybody (which is the normal case except when
Linus creates these horribly involved special cases that probably never
happen in real life), d_delete() would have turned the "huh" entry into
a negative dentry.

The good news is:
- I thought about this horrible case when I wrote the code.
- we're able to re-use code (d_delete works fine for us)
- the default case is the really efficient one that does exactly what we
want (it creates a negative dentry for the name that got removed, with
close to zero overhead, so the dcache now contains more information
that we were able to glean from the fact that we moved an entry).

The bad news is:
- "cat /proc/<xxx>/fd/<foo>" will report "foo" to be named "huh", because
in the above horror-example we couldn't do the thing we wanted (which
was to turn it into a negative dentry). This is ugly, because the file
that we point to with "foo" has never ever actually been named "huh" -
it only got overwritten with a file called "huh".

However. The bad news isn't really bad (there is no real path that we
could follow to "foo" anyway, so in some sense any path is as good as any
other path). And the good news is that the new code is extremely efficient
and simple, and gives exactly the semantics we want apart from this one
very slight strange quirk.

If somebody wants to, the d_move() code could be re-written to:
- if we can do the negative dentry thing (ie "target->d_count == 1" in
d_move) then we use the current algorithm.
- if "target" has users, we can duplicate the name, and do the thing it
used to do.

However, I'm certain that I personally don't want to create the complex
code to do something that I don't feel is all that much of a win anyway.
The alternative (suggested by Alan) is to make /proc simply not show
deleted paths (this is pretty trivial to do: you can decide whether a
dentry has been deleted with this simple logic:

int is_deleted(struct dentry * dentry)
{
return dentry->d_parent != dentry &&
list_empty(&dentry->d_hash);
}

which is again totally untested but should work (the logic is trivial and
obvious: if a dentry has a parent but is not hashed, then it has been
deleted).

> Also, please consider the changes to d_alloc, d_add, and real_lookup in
> my patch. These keep the dentry tree in a safe state so we don't have
> to worry about the fs blocking after d_add, or corrupting the dcache by
> not d_add()ing but not returning error.

Can you re-send only that part of the patch, I got sidetracked by the
d_move() change that I preferred doing my way instead?

Linus


\
 
 \ /
  Last update: 2005-03-22 13:40    [W:0.070 / U:0.100 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site