Re: [PATCH 2/3] Retry page fault when blocking on disk transfer.

[Michel Lespinasse]

On Fri, Oct 8, 2010 at 6:24 AM, Rik van Riel <riel@xxxxxxxxxx> wrote:
>> +static inline int lock_page_or_retry(struct page *page, struct mm_struct
>> *mm,
>> +                                    unsigned int flags)
>> +{
>> +       if (trylock_page(page))
>> +               return 1;
>> +       if (!(flags&  FAULT_FLAG_ALLOW_RETRY)) {
>> +               __lock_page(page);
>> +               return 1;
>> +       }
>> +
>> +       up_read(&mm->mmap_sem);
>> +       wait_on_page_locked(page);
>> +       return 0;
>> +}
>
> Wait a moment.  Your other patch 2/3 also has a
> lock_page_or_retry function.  That one is in
> filemap.c and takes slightly different arguments,
> to do essentially the same thing...
>
> +/*
> + * Lock the page, unless this would block and the caller indicated that it
> + * can handle a retry.
> + */
> +static int lock_page_or_retry(struct page *page,
> +                             struct vm_area_struct *vma, struct vm_fault
> *vmf)
> +{
>
> Is there a way the two functions can be merged
> into one?

Yes, this would be easy to do.

The argument against it would be loss of inlining and, in the filemap
version, the need to reference vma and vmf fields to find out the mm
and flags values. We'd like to avoid doing that at least in the fast
path when trylock_page succeeds - though, now that I think about it,
both could be avoided with an inline function in a header returning
trylock_page(page) || _lock_page_or_retry(mm, flags)

Hmmm, this is actually quite similar to how other functions in
pagemap.h / filemap.c are done...
I'll send an updated series using this suggestion.

-- 
Michel "Walken" Lespinasse
A program is never fully debugged until the last user dies.
--
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/