On Thu, 19 May 2011, Minchan Kim wrote:
> On Thu, May 19, 2011 at 9:28 AM, Hugh Dickins <hughd@xxxxxxxxxx> 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