On Thu, 19 May 2011, Minchan Kim wrote:

On Thu, May 19, 2011 at 9:28 AM, Hugh Dickins [off-list ref] wrote:

quoted

On Thu, 19 May 2011, Minchan Kim wrote:

quoted

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