All of lore.kernel.org
 help / color / mirror / Atom feed
From: Omar Sandoval <osandov@osandov.com>
To: Trond Myklebust <trond.myklebust@primarydata.com>
Cc: Alexander Viro <viro@zeniv.linux.org.uk>,
	Andrew Morton <akpm@linux-foundation.org>,
	Christoph Hellwig <hch@infradead.org>,
	David Sterba <dsterba@suse.cz>,
	linux-fsdevel@vger.kernel.org, linux-mm@kvack.org,
	linux-nfs@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH 1/8] nfs: follow direct I/O write locking convention
Date: Mon, 15 Dec 2014 07:42:03 -0800	[thread overview]
Message-ID: <20141215154203.GA20161@mew> (raw)
In-Reply-To: <CAABAsM4jMcox1emR1nSxORUOPNMDYmCcmMD4YymJ9R_BM_UU4w@mail.gmail.com>

On Mon, Dec 15, 2014 at 07:49:20AM -0500, Trond Myklebust wrote:
> On Mon, Dec 15, 2014 at 12:26 AM, Omar Sandoval <osandov@osandov.com> wrote:
> > The generic callers of direct_IO lock i_mutex before doing a write. NFS
> > doesn't use the generic write code, so it doesn't follow this
> > convention. This is now a problem because the interface introduced for
> > swap-over-NFS calls direct_IO for a write without holding i_mutex, but
> > other implementations of direct_IO will expect to have it locked.
> 
> I really don't care much about swap-over-NFS performance; that's a
> niche usage at best. I _do_ care about O_DIRECT performance, and the
> ability to run multiple WRITE calls in parallel.
> 
> IOW: Patch NACKed... Please find another solution.
> 
> Trond

So the patch formatting doesn't make it completely clear what's going on
here, but here's what the original nfs_file_direct_write code did:

- called with i_mutex unlocked
- collects stats and does some generic checks
- locks i_mutex
- syncs the mapping, schedules the write
- unlocks i_mutex
- waits for the write to complete if synchronous

After this patch, nfs_file_direct_write works like:

- called with i_mutex locked
- collects stats and does some generic checks
- syncs the mapping, schedules the write
- drops i_mutex
- waits for the write to complete if synchronous
- picks i_mutex back up

There's an extra lock and unlock as a result and a slightly longer
critical section, but we drop i_mutex to wait for the write, so multiple
writes still work in parallel.

-- 
Omar

WARNING: multiple messages have this Message-ID (diff)
From: Omar Sandoval <osandov@osandov.com>
To: Trond Myklebust <trond.myklebust@primarydata.com>
Cc: Alexander Viro <viro@zeniv.linux.org.uk>,
	Andrew Morton <akpm@linux-foundation.org>,
	Christoph Hellwig <hch@infradead.org>,
	David Sterba <dsterba@suse.cz>,
	linux-fsdevel@vger.kernel.org, linux-mm@kvack.org,
	linux-nfs@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH 1/8] nfs: follow direct I/O write locking convention
Date: Mon, 15 Dec 2014 07:42:03 -0800	[thread overview]
Message-ID: <20141215154203.GA20161@mew> (raw)
In-Reply-To: <CAABAsM4jMcox1emR1nSxORUOPNMDYmCcmMD4YymJ9R_BM_UU4w@mail.gmail.com>

On Mon, Dec 15, 2014 at 07:49:20AM -0500, Trond Myklebust wrote:
> On Mon, Dec 15, 2014 at 12:26 AM, Omar Sandoval <osandov@osandov.com> wrote:
> > The generic callers of direct_IO lock i_mutex before doing a write. NFS
> > doesn't use the generic write code, so it doesn't follow this
> > convention. This is now a problem because the interface introduced for
> > swap-over-NFS calls direct_IO for a write without holding i_mutex, but
> > other implementations of direct_IO will expect to have it locked.
> 
> I really don't care much about swap-over-NFS performance; that's a
> niche usage at best. I _do_ care about O_DIRECT performance, and the
> ability to run multiple WRITE calls in parallel.
> 
> IOW: Patch NACKed... Please find another solution.
> 
> Trond

So the patch formatting doesn't make it completely clear what's going on
here, but here's what the original nfs_file_direct_write code did:

- called with i_mutex unlocked
- collects stats and does some generic checks
- locks i_mutex
- syncs the mapping, schedules the write
- unlocks i_mutex
- waits for the write to complete if synchronous

After this patch, nfs_file_direct_write works like:

- called with i_mutex locked
- collects stats and does some generic checks
- syncs the mapping, schedules the write
- drops i_mutex
- waits for the write to complete if synchronous
- picks i_mutex back up

There's an extra lock and unlock as a result and a slightly longer
critical section, but we drop i_mutex to wait for the write, so multiple
writes still work in parallel.

-- 
Omar

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

  reply	other threads:[~2014-12-15 15:42 UTC|newest]

Thread overview: 63+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2014-12-15  5:26 [PATCH 0/8] clean up and generalize swap-over-NFS Omar Sandoval
2014-12-15  5:26 ` Omar Sandoval
2014-12-15  5:26 ` [PATCH 1/8] nfs: follow direct I/O write locking convention Omar Sandoval
2014-12-15  5:26   ` Omar Sandoval
2014-12-15 12:49   ` Trond Myklebust
2014-12-15 12:49     ` Trond Myklebust
2014-12-15 15:42     ` Omar Sandoval [this message]
2014-12-15 15:42       ` Omar Sandoval
2014-12-15  5:26 ` [PATCH 2/8] swap: lock i_mutex for swap_writepage direct_IO Omar Sandoval
2014-12-15  5:26   ` Omar Sandoval
2014-12-15 16:27   ` Jan Kara
2014-12-15 16:27     ` Jan Kara
2014-12-15 16:56     ` Christoph Hellwig
2014-12-15 16:56       ` Christoph Hellwig
2014-12-15 22:11       ` Omar Sandoval
2014-12-15 22:11         ` Omar Sandoval
2014-12-16  8:35         ` Christoph Hellwig
2014-12-16  8:35           ` Christoph Hellwig
2014-12-16  8:35           ` Christoph Hellwig
2014-12-16  8:56           ` Omar Sandoval
2014-12-16  8:56             ` Omar Sandoval
2014-12-17  8:06             ` Christoph Hellwig
2014-12-17  8:06               ` Christoph Hellwig
2014-12-17  8:20               ` Al Viro
2014-12-17  8:20                 ` Al Viro
2014-12-17  8:24                 ` Christoph Hellwig
2014-12-17  8:24                   ` Christoph Hellwig
2014-12-17 14:58                   ` Omar Sandoval
2014-12-17 14:58                     ` Omar Sandoval
2014-12-17 18:52                     ` Christoph Hellwig
2014-12-17 18:52                       ` Christoph Hellwig
2014-12-17 22:03                       ` Al Viro
2014-12-17 22:03                         ` Al Viro
2014-12-19  6:24                         ` Omar Sandoval
2014-12-19  6:24                           ` Omar Sandoval
2014-12-19  6:28                           ` Al Viro
2014-12-19  6:28                             ` Al Viro
2014-12-19  6:28                             ` Al Viro
2014-12-20  6:51       ` Al Viro
2014-12-20  6:51         ` Al Viro
2014-12-22  7:26         ` Omar Sandoval
2014-12-22  7:26           ` Omar Sandoval
2014-12-23  9:37         ` Christoph Hellwig
2014-12-23  9:37           ` Christoph Hellwig
2014-12-23  9:37           ` Christoph Hellwig
2014-12-15  5:26 ` [PATCH 3/8] swap: don't add ITER_BVEC flag to direct_IO rw Omar Sandoval
2014-12-15  5:26   ` Omar Sandoval
2014-12-15  6:16   ` Al Viro
2014-12-15  6:16     ` Al Viro
2014-12-15 15:57     ` Omar Sandoval
2014-12-15 15:57       ` Omar Sandoval
2014-12-15  5:26 ` [PATCH 4/8] iov_iter: add iov_iter_bvec and convert callers Omar Sandoval
2014-12-15  5:26   ` Omar Sandoval
2014-12-15  5:26 ` [PATCH 5/8] direct-io: don't dirty ITER_BVEC pages on read Omar Sandoval
2014-12-15  5:26   ` Omar Sandoval
2014-12-15  5:27 ` [PATCH 6/8] nfs: don't dirty ITER_BVEC pages read through direct I/O Omar Sandoval
2014-12-15  5:27   ` Omar Sandoval
2014-12-15  6:17   ` Al Viro
2014-12-15  6:17     ` Al Viro
2014-12-15  5:27 ` [PATCH 7/8] swap: use direct I/O for SWP_FILE swap_readpage Omar Sandoval
2014-12-15  5:27   ` Omar Sandoval
2014-12-15  5:27 ` [PATCH 8/8] vfs: update swap_{,de}activate documentation Omar Sandoval
2014-12-15  5:27   ` Omar Sandoval

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20141215154203.GA20161@mew \
    --to=osandov@osandov.com \
    --cc=akpm@linux-foundation.org \
    --cc=dsterba@suse.cz \
    --cc=hch@infradead.org \
    --cc=linux-fsdevel@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mm@kvack.org \
    --cc=linux-nfs@vger.kernel.org \
    --cc=trond.myklebust@primarydata.com \
    --cc=viro@zeniv.linux.org.uk \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.