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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



> From: "Matthew Wilcox" <matthew@xxxxxx>
> To: "Alexandre Depoutovitch" <adepoutovitch@xxxxxxxxxx>
> Cc: linux-nfs@xxxxxxxxxxxxxxx, linux-fsdevel@xxxxxxxxxxxxxxx
> Sent: Monday, April 30, 2012 3:56:46 PM
> Subject: Re: [PATCH RFC v2] Performing direct I/O on sector-aligned requests
> 
> 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.


Thank you, I will do this.

 
> > +       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.


I have already changed this. Thanks.
 
> > +           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.


I will remove it if is 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.



I copy/pasted it from the previous entry in the same file :(

 
> --
> 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."
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[Index of Archives]     [Linux Ext4 Filesystem]     [Union Filesystem]     [Filesystem Testing]     [Ceph Users]     [Ecryptfs]     [AutoFS]     [Kernel Newbies]     [Share Photos]     [Security]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux Cachefs]     [Reiser Filesystem]     [Linux RAID]     [Samba]     [Device Mapper]     [CEPH Development]
  Powered by Linux