Re: [PATCH 0/3] mm: tracking dirty pages -v5

[Peter Zijlstra]

On Tue, 2006-06-06 at 21:06 +0100, Hugh Dickins wrote:

> You tend to use get_page/put_page amidst code using page_cache_get/
> page_cache_release.  Carry on: it sometimes looks odd, but I can't see
> any way to impose consistency, short of abolishing one or the other
> throughout the tree.  So don't worry about it.

Noticed that myself too, came to the same conclusion, thanks for the
confirmation thought.

> You've got a minor cleanup to install_page, left over from an earlier
> iteration: the cleanup looked okay, but of no relevance to your patchset
> now, is it?  Just cut mm/fremap.c out of the patchset I think.

OK, unless we go back to the previous way I'll send this tiny cleanup as
a separate patch to Andrew.

> You've taken the simplification of sys_msync a little too far, I believe:
> you ought to try to reproduce the same errors as before, so MS_ASYNC
> should be winding through the separate vmas like MS_SYNC, just to
> report -ENOMEM if it crosses an unmapped area; and MS_INVALIDATE
> used to say -EBUSY if VM_LOCKED, but that has disappeared.  (Perhaps
> I've missed other such details, please recheck.)

Ah, yes, I've noticed this too, I just wasn't sure on if this would be
wanted or not. Is fixed, thanks!

> Your comment should
> say "Nor does it mark" instead of "Nor does it just marks".

Hehe, thanks, please do point out my mistakes with the English language.
I have good enough control to convey most of what I intent but I'm not a
native.

> Your is_shared_writable(vma) in mprotect_fixup is along the right
> lines, but wrong: because at that point vma->vm_flags is the old one,
> and may be omitting VM_WRITE when that is about to be added.  Perhaps
> you should move the "vma->vm_flags = newflags" above it, or perhaps
> you should change is_shared_writable to work on flags rather than vma
> (as Linus' is_cow_mapping does).

How odd, I have the distinct recollection of actually moving that
assignment upwards a few lines, must have been late or something. Thanks
for pointing this out, would've never found it again; with me thinking
it was done with.

<snip: big tiresome story/>

Well, you got me. I don't know either, the only thing I can come up with
is making the breakage compile-time (for 3rd-party modules) instead of
subtle run-time, but its still not pretty.

/me looks around at assorted VM gurus; any ideas out there?

If by tomorrow morning CET nobody has spoken up, I'll just go ahead and
accept Hugh's apology :-), that is revert back to my original way of
doing it. (I can always go back to this scheme if some smart but slower
working brain manages a solution)


Peter

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/