Re: [PATCH mmotm] add the pagefault count into memcg stats: shmem fix

[Hugh Dickins <hughd@google.com>]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1983 bytes --]

On Thu, 19 May 2011, Minchan Kim wrote:
> On Thu, May 19, 2011 at 9:28 AM, Hugh Dickins <hughd@google.com> wrote:
> > On Thu, 19 May 2011, Minchan Kim wrote:
> >>
> >> You are changing behavior a bit.
> >> Old behavior is to account FAULT although the operation got failed.
> >> But new one is to not account it.
> >> I think we have to account it regardless of whether it is successful or not.
> >> That's because it is fact fault happens.
> >
> > That's a good catch: something I didn't think of at all.
> >
> > However, it looks as if the patch remains correct, and is fixing
> > a bug (or inconsistency) that we hadn't noticed before.
> >
> > If you look through filemap_fault() or do_swap_page() (or even
> > ncp_file_mmap_fault(), though I don't take that one as canonical!),
> > they clearly do not count the major fault on error (except in the
> > case where VM_FAULT_MAJOR needs VM_FAULT_RETRY, then gets
> > VM_FAULT_ERROR on the retry).
> >
> > So, shmem.c was the odd one out before.  If you feel very strongly
> > about it ("it is fact fault happens") you could submit a patch to
> > change them all - but I think just leave them as is.
> 
> Okay. I don't feel it strongly now.
> Then, could you repost your patch with corrected description about
> this behavior change which is a bug or inconsistency whatever. :)

If I can think up a correct paragraph which makes it clear.
Let me hold off on that, and see what other comments come in.

The situation is less clear-cut than I described above.  I was
dismissing the VM_FAULT_MAJOR|VM_FAULT_RETRY then VM_FAULT_ERROR
case as unlikely, whereas that would be the common case of I/O error
on fault on x86 now - reading in takes page lock, so it will retry.

Whereas other architectures (not using FAULT_FLAG_ALLOW_RETRY)
behave as I described, as filemap_fault() used to behave.

I don't think this a major fault in my patch ;)
Just something we don't care very much about.

Hugh