Re: [patch] mm: bug in set_page_dirty_buffers

[Peter Zijlstra <a.p.zijlstra@chello.nl>]

On Tue, 2006-10-10 at 08:59 +0200, Nick Piggin wrote:
> On Tue, Oct 10, 2006 at 08:48:56AM +0200, Peter Zijlstra wrote:
> > On Tue, 2006-10-10 at 07:22 +0200, Nick Piggin wrote:
> > > 
> > > I disagree because it will lead to horrible hacks because many callers
> > > can't sleep. If anything I would much prefer an innermost-spinlock in
> > > page->flags that specifically excludes truncate. Actually tree_lock can
> > > do that now, provided we pin mapping in all callers to set_page_dirty
> > > (which we should do).
> > 
> > Yeah, but we're hard working to eradicate tree lock; I have ran into
> 
> Well yeah, but until then the tree_lock works.

True, and your latest patch looks nice, will have to go over it in more
than a cursory glance though.

> > this problem before; that is, zap_pte_range and co. not being able to
> > lock the page. I'd really like to see that fixed.
> 
> What's your problem with zap_pte_range?

well, not only zap_pte_range, also page_remove_rmap and
try_to_unmap_cluster etc.. Basically all those who fiddle with the page
without holding the page lock.

Because with concurrent pagecache, there is no tree lock anymore to
protect/pin the whole mapping, I need to go pin individual pages.
Perhaps having an inner page bit-spinlock (PG_pin) isn't a bad thing,
I'd just raised the issue to see if it would be doable/a-good-thing to
try and merge these two.

> > In my current concurrent pagecache patches I've abused your PG_nonewrefs
> > and made it this page internal (bit)spinlock, but it just doesn't look
> > nice to have both this lock and PG_locked.
> 
> Regardless of whether or not they spin, PG_locked is an outermost, and
> set_page_dirty is called innermost. 

Yes, indeed, and changing it is going to be hard and painful, hence I'll
stick with PG_pin for now.

> I don't see why we'd particularly
> want to mush them together now, just because we're worried a filesystem
> writer wrote buggy code.
> 
> It is all well and good to tell me I'm wrong unless I audit all
> filesystems, but the fact is that I'm not a filesystem expert, and if
> this is what it has come to then either the process has failed, or
> we have a large number of filesystems to cull from the tree as
> unmaintained.

With you on that, no/little filesystems knowledge here either.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>