Patch "fs: use RCU read side protection in d_validate" broken

Patch "fs: use RCU read side protection in d_validate" broken

[Nick Piggin]

This patch is totally broken. You can't just dget() a dentry with
nothing but RCU critical section open.

The patch in my tree this is claimed to be split out of, at least
attemptet to do some locking, I don't know why that was stripped
out. But I didn't get that quite right myself at which point I
decided to just forget about it entirely.

Christoph, why did you think such a patch is worth getting merged, btw?
I saw no hint of a justification in your changelog.  I mean, in my tree
at least there was a _rationale_ that dcache_lock is going away and this
marginally made the locking simpler. But it doesn't make sense in the
current tree, even if the merged patch was _not_ buggy -- what were you
trying to do, make ncpfs's readdir go really fast?

Breaking things out when they make sense and stand as patches on their
own. That is very important. When they do _not_ make sense on their own,
then that is when they do not make sense to be broken out and merged
ahead of their series.

Linus, I cooked up a fix for it, but decided it's stupid to bother with
any complexity here. Please revert
3825bdb7ed920845961f32f364454bee5f469abb

If I end up wanting it again for dcache series, I can do it with proper
locking and proper justification.

Thanks,
Nick

Re: Patch "fs: use RCU read side protection in d_validate" broken

[Nick Piggin]

On Mon, Nov 15, 2010 at 04:11:20PM +1100, Nick Piggin wrote:
> This patch is totally broken. You can't just dget() a dentry with
> nothing but RCU critical section open.
> 
> The patch in my tree this is claimed to be split out of, at least
> attemptet to do some locking, I don't know why that was stripped
> out. But I didn't get that quite right myself at which point I
> decided to just forget about it entirely.
> 
> Christoph, why did you think such a patch is worth getting merged, btw?
> I saw no hint of a justification in your changelog.  I mean, in my tree
> at least there was a _rationale_ that dcache_lock is going away and this
> marginally made the locking simpler. But it doesn't make sense in the
> current tree, even if the merged patch was _not_ buggy -- what were you
> trying to do, make ncpfs's readdir go really fast?

Also, could you try to have a bit more common sense, in general, with
these things?

I mean, your dentry lru modification patch really didn't need to be
pulled ahead of my other patches and and subtly changed. That just
scatters wreckage throughout my patchset, which goes beyond just
merging things up but also all the stress testing and verification I've
done goes out the window too.

Yes, I may not have the thing structured *exactly* as you want it, but
really, unless it is a real problem, just look at the big picture a bit
more.

Re: Patch "fs: use RCU read side protection in d_validate" broken

[Christoph Hellwig]

On Mon, Nov 15, 2010 at 04:32:30PM +1100, Nick Piggin wrote:
> I mean, your dentry lru modification patch really didn't need to be
> pulled ahead of my other patches and and subtly changed. That just
> scatters wreckage throughout my patchset, which goes beyond just
> merging things up but also all the stress testing and verification I've
> done goes out the window too.

There's a lot more dcache cleanups that need to go in before we can
do the lock splitting in a sensible way.  I have started doing that a
couple of weeks ago while you were away.  I've been keeping this back
in the hope that we could the mud fight and get back to the table
working together.

Any good way to encourage you to stick to the techical feedback instead
of two or three flames in reply to any disagreement by others?  Also
I'd be very happy you could stop sending me personal accusations of
a troll.  

> Yes, I may not have the thing structured *exactly* as you want it, but
> really, unless it is a real problem, just look at the big picture a bit
> more.

In VFS land we've done pretty well with doing cleanups before locking
or algorithm changes to make them smaller and better to audit.  It's not
just my opinion, ask Al for his even more fine grained split up request
for the inode_lock splitup.  I think splitting things into these small
blocks and moving the cleanup bits to the beginning has helped that code
a lot.  We found a couple of bugs, both in the initial patches and the
later version, and the final patches are very easy to understand and
verify.

Yes, it is a lot more work, but the result does pay off.

Re: Patch "fs: use RCU read side protection in d_validate" broken

[Nick Piggin]

On Mon, Nov 15, 2010 at 10:16:08PM +0100, Christoph Hellwig wrote:
> On Mon, Nov 15, 2010 at 04:32:30PM +1100, Nick Piggin wrote:
> > I mean, your dentry lru modification patch really didn't need to be
> > pulled ahead of my other patches and and subtly changed. That just
> > scatters wreckage throughout my patchset, which goes beyond just
> > merging things up but also all the stress testing and verification I've
> > done goes out the window too.
> 
> There's a lot more dcache cleanups that need to go in before we can
> do the lock splitting in a sensible way.  I have started doing that a
> couple of weeks ago while you were away.  I've been keeping this back
> in the hope that we could the mud fight and get back to the table
> working together.

My tree is obviously there, I've been wanting reviews and suggestions
for months.

 
> Any good way to encourage you to stick to the techical feedback instead
> of two or three flames in reply to any disagreement by others?  Also

Yes, I'll stick to the actual technical feedback because I've decided
to ignore all the other crap. By now they seem immune to flames, so it's
pointless.


> I'd be very happy you could stop sending me personal accusations of
> a troll.  

Like when I explained (for the Nth time) why SLAB_DESTROY_BY_RCU was
difficult for rcu-walk, and not much point for inode hash lookup, which
you then ignored and posted your usual wrong FUD?

 "Dave sent a patch for it, which looks much better to me.  Nick thinks
  it doesn't work for his store free path walk, but I haven't seen an
  explanation why exactly."

Any good way to encourage you to actually follow what's going on, and
maybe *read* my emails and give me the benefit of the doubt instead of
assuming I'm wrong?


> > Yes, I may not have the thing structured *exactly* as you want it, but
> > really, unless it is a real problem, just look at the big picture a bit
> > more.
> 
> In VFS land we've done pretty well with doing cleanups before locking
> or algorithm changes to make them smaller and better to audit.  It's not
> just my opinion, ask Al for his even more fine grained split up request
> for the inode_lock splitup.  I think splitting things into these small
> blocks and moving the cleanup bits to the beginning has helped that code
> a lot.  We found a couple of bugs, both in the initial patches and the
> later version, and the final patches are very easy to understand and
> verify.
> 
> Yes, it is a lot more work, but the result does pay off.

I know, I'm not saying they're always wrong. But there are always cleanups
to do, and some cleanup patches which don't do much to help a bigger pending
transformation can be just as easily put after such a work.

Re: Patch "fs: use RCU read side protection in d_validate" broken

[Christoph Hellwig]

On Mon, Nov 15, 2010 at 04:11:20PM +1100, Nick Piggin wrote:
> This patch is totally broken. You can't just dget() a dentry with
> nothing but RCU critical section open.

The plain dget is indeed wrong as we should at least take d_lock
and check d_count for zero before incrementing it to protect
against shrink_dentry_list.  

I'm not quite sure it really matters as d_validate already has
and always ad much worse bugs, such as the complete lack of
protection against renames.

Anyway, I'll send a patch to Linus to fix this issue for now.

Re: Patch "fs: use RCU read side protection in d_validate" broken

[Nick Piggin]

On Mon, Nov 15, 2010 at 10:09:06PM +0100, Christoph Hellwig wrote:
> On Mon, Nov 15, 2010 at 04:11:20PM +1100, Nick Piggin wrote:
> > This patch is totally broken. You can't just dget() a dentry with
> > nothing but RCU critical section open.
> 
> The plain dget is indeed wrong as we should at least take d_lock
> and check d_count for zero before incrementing it to protect
> against shrink_dentry_list.  
> 
> I'm not quite sure it really matters as d_validate already has

Explain why it doesn't matter. It's an oopsable bug introduced.


> and always ad much worse bugs, such as the complete lack of
> protection against renames.

What are the much worse bugs? What do you mean by rename protection?


> Anyway, I'll send a patch to Linus to fix this issue for now.

A revert is appropriate. Like I said, there is no need for this patch at
all and no justification provided.