From mboxrd@z Thu Jan 1 00:00:00 1970 Subject: Re: [PATCH 0/3] mm: tracking dirty pages -v5 From: Peter Zijlstra In-Reply-To: References: <20060525135534.20941.91650.sendpatchset@lappy> Content-Type: text/plain Date: Wed, 07 Jun 2006 20:08:50 +0200 Message-Id: <1149703730.4408.45.camel@lappy> Mime-Version: 1.0 Content-Transfer-Encoding: 7bit Sender: owner-linux-mm@kvack.org Return-Path: To: Hugh Dickins Cc: linux-mm@kvack.org, linux-kernel@vger.kernel.org, Andrew Morton , David Howells , Christoph Lameter , Martin Bligh , Nick Piggin , Linus Torvalds List-ID: 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. 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, 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: email@kvack.org