Re: [PATCH RFC v2] Performing direct I/O on sector-aligned requests

[Matthew Wilcox <matthew@wil.cx>]

On Sun, Apr 29, 2012 at 02:03:41PM -0700, Alexandre Depoutovitch wrote:
> +   if (current->mm) {
> +       ret = get_user_pages_fast(
> +           dio->curr_user_address,     /* Where from? */
> +           nr_pages,           /* How many pages? */
> +           dio->rw == READ,        /* Write to memory? */
> +           &dio->pages[0]);        /* Put results here */
> +   } else {
> +       /* For kernel threads mm is NULL, so all we need is to increment
> +       page's reference count and add page to dio->pages array */
> +       int i;
> +       struct page* page;
> +       unsigned long start_pfn = virt_to_phys((void 
> *)dio->curr_user_address)
> +           >> PAGE_SHIFT;
> +       /* For kernel threads buffer must be in kernel memory */
> +       BUG_ON(dio->curr_user_address < TASK_SIZE_MAX);

This is an assumption that isn't true for all architectures.  Better just
delete this line.

> +       for (i = 0; i < nr_pages; i++) {
> +           page = pfn_to_page(start_pfn + i);

Why are you messing about with pfns?  Why not just stay with virtual
addresses and call virt_to_page() in this loop?  That would ensure that
this works to vmapped pages as well as physically contiguous pages.

> +           page_cache_get(page);
> +           dio->pages[i] = page;
> +       }
> +       /* No need to lock pages: this is kernel thread and the pages are in
> +         kernel as well */
> +       ret = nr_pages;
> +   }
> 
>     if (ret < 0 && dio->blocks_available && (dio->rw & WRITE)) {
>         struct page *page = ZERO_PAGE(0);
> @@ -972,7 +991,11 @@
>                 break;
>         }
> 
> -       /* Drop the ref which was taken in get_user_pages() */
> +       /*
> +        * Drop the ref which was taken in dio_refill_pages
> +        * directly (for direct I/O) or by calling get_user_pages
> +        * (for buffered IO)
> +        */

I think your change to this comment actually makes it more confusing.

> @@ -1348,6 +1351,58 @@
>                             nfsd_max_blksize);
>  }
> 
> +int nfsd_directio_mode = DIO_NEVER;
> +
> +/**
> + * nfsd_directio_mode - sets conditions when direct IO is activated
> + *
> + * Input:
> + *         buf:        ignored
> + *         size:       zero
> + *
> + * OR
> + *
> + * Input:
> + *             buf:        C string containing an unsigned
> + *                     integer value representing the new
> + *                     NFS direct IO mode
> + *         size:       non-zero length of C string in @buf
> + * Output:
> + * On success: passed-in buffer filled with '\n'-terminated C string
> + *         containing numeric value of the current direct IO mode
> + *         return code is the size in bytes of the string
> + *
> + * Possible modes are:
> + *     DIO_NEVER (0) - never use direct I/O
> + *     DIO_FS_UNALIGNED (1) - use direct I/O only for requests that FS 
> unaligned
> + *         and block device aligned
> + *     DIO_SECTOR_ALIGNED (3) - use direct I/O for all block device aligned 
> IO
> + * On error:   return code is zero or a negative errno value
> + */

This is not correct kerneldoc formatting.

-- 
Matthew Wilcox				Intel Open Source Technology Centre
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."