On Thu, 08 Mar 2018, Trond Myklebust wrote:
> On Thu, 2018-03-08 at 08:09 -0500, Scott Mayhew wrote:
> > Yes, this works. I ran it through a dozen fio runs on v4.1 and 1000
> > runs
> > of generic/247 on v3/v4.0/v4.1/v4.2 and didn't see any EBUSY errors.
> > Also ran the xfstests "quick" group (~80-90 tests) plus generic/074
> > on
> > v3/v4.0/v4.1/v4.2. Finally, I double checked the panic on umount
> > issue
> > that dc4fd9ab01ab3 fixed and that still works too.
>
> I took a long hard look at what we actually need in that area of the
> code. There are a few things that are still broken there:
>
> Firstly, we want to keep the inode marked as I_DIRTY_DATASYNC as long
> as we have stable writes that are undergoing commit or are waiting to
> be scheduled. The reason is that ensures sync_inode() behaves correctly
> by calling into nfs_write_inode() so that we can schedule COMMITs and
> wait for them all to complete.
> Currently we are broken in that nfs_write_inode() will not reset
> I_DIRTY_DATASYNC if there are still COMMITs in flight due to having
> called it with wbc->sync_mode == WB_SYNC_NONE.
>
> Secondly, we want to ensure that if the number of requests is >
> INT_MAX, we loop around and schedule more COMMITs so that
> nfs_commit_inode(inode, FLUSH_SYNC) is reliable on systems with lots of
> memory.
>
> Finally, it is worth noting that it's only when called from
> __writeback_single_inode(), and the attempt to clean the inode failed
> that we need to reset the inode state. So we can optimise by pushing
> those calls to __mark_inode_dirty() into nfs_write_inode().
>
> So how about the following v2 patch instead?
> 8<--------------------------------------------
> From 386978cc3ef4494b9f95390747c2268f8318b94b Mon Sep 17 00:00:00 2001
> From: Trond Myklebust <trond.myklebust@xxxxxxxxxxxxxxx>
> Date: Wed, 7 Mar 2018 15:22:31 -0500
> Subject: [PATCH v2] NFS: Fix unstable write completion
>
> We do want to respect the FLUSH_SYNC argument to nfs_commit_inode() to
> ensure that all outstanding COMMIT requests to the inode in question are
> complete. Currently we may exit early from both nfs_commit_inode() and
> nfs_write_inode() even if there are COMMIT requests in flight, or unstable
> writes on the commit list.
>
> In order to get the right semantics w.r.t. sync_inode(), we don't need
> to have nfs_commit_inode() reset the inode dirty flags when called from
> nfs_wb_page() and/or nfs_wb_all(). We just need to ensure that
> nfs_write_inode() leaves them in the right state if there are outstanding
> commits, or stable pages.
>
> Reported-by: Scott Mayhew <smayhew@xxxxxxxxxx>
> Fixes: dc4fd9ab01ab ("nfs: don't wait on commit in nfs_commit_inode()...")
> Cc: stable@xxxxxxxxxxxxxxx # v4.5+: 5cb953d4b1e7: NFS: Use an atomic_long_t
> Cc: stable@xxxxxxxxxxxxxxx # v4.5+
> Signed-off-by: Trond Myklebust <trond.myklebust@xxxxxxxxxxxxxxx>
I ran all the same tests as before and this is working fine.
-Scott
> ---
> fs/nfs/write.c | 83 ++++++++++++++++++++++++++++++----------------------------
> 1 file changed, 43 insertions(+), 40 deletions(-)
>
> diff --git a/fs/nfs/write.c b/fs/nfs/write.c
> index 7428a669d7a7..e7d8ceae8f26 100644
> --- a/fs/nfs/write.c
> +++ b/fs/nfs/write.c
> @@ -1876,40 +1876,43 @@ int nfs_generic_commit_list(struct inode *inode, struct list_head *head,
> return status;
> }
>
> -int nfs_commit_inode(struct inode *inode, int how)
> +static int __nfs_commit_inode(struct inode *inode, int how,
> + struct writeback_control *wbc)
> {
> LIST_HEAD(head);
> struct nfs_commit_info cinfo;
> int may_wait = how & FLUSH_SYNC;
> - int error = 0;
> - int res;
> + int ret, nscan;
>
> nfs_init_cinfo_from_inode(&cinfo, inode);
> nfs_commit_begin(cinfo.mds);
> - res = nfs_scan_commit(inode, &head, &cinfo);
> - if (res)
> - error = nfs_generic_commit_list(inode, &head, how, &cinfo);
> + for (;;) {
> + ret = nscan = nfs_scan_commit(inode, &head, &cinfo);
> + if (ret <= 0)
> + break;
> + ret = nfs_generic_commit_list(inode, &head, how, &cinfo);
> + if (ret < 0)
> + break;
> + ret = 0;
> + if (wbc && wbc->sync_mode == WB_SYNC_NONE) {
> + if (nscan < wbc->nr_to_write)
> + wbc->nr_to_write -= nscan;
> + else
> + wbc->nr_to_write = 0;
> + }
> + if (nscan < INT_MAX)
> + break;
> + cond_resched();
> + }
> nfs_commit_end(cinfo.mds);
> - if (res == 0)
> - return res;
> - if (error < 0)
> - goto out_error;
> - if (!may_wait)
> - goto out_mark_dirty;
> - error = wait_on_commit(cinfo.mds);
> - if (error < 0)
> - return error;
> - return res;
> -out_error:
> - res = error;
> - /* Note: If we exit without ensuring that the commit is complete,
> - * we must mark the inode as dirty. Otherwise, future calls to
> - * sync_inode() with the WB_SYNC_ALL flag set will fail to ensure
> - * that the data is on the disk.
> - */
> -out_mark_dirty:
> - __mark_inode_dirty(inode, I_DIRTY_DATASYNC);
> - return res;
> + if (ret || !may_wait)
> + return ret;
> + return wait_on_commit(cinfo.mds);
> +}
> +
> +int nfs_commit_inode(struct inode *inode, int how)
> +{
> + return __nfs_commit_inode(inode, how, NULL);
> }
> EXPORT_SYMBOL_GPL(nfs_commit_inode);
>
> @@ -1919,11 +1922,11 @@ int nfs_write_inode(struct inode *inode, struct writeback_control *wbc)
> int flags = FLUSH_SYNC;
> int ret = 0;
>
> - /* no commits means nothing needs to be done */
> - if (!atomic_long_read(&nfsi->commit_info.ncommit))
> - return ret;
> -
> if (wbc->sync_mode == WB_SYNC_NONE) {
> + /* no commits means nothing needs to be done */
> + if (!atomic_long_read(&nfsi->commit_info.ncommit))
> + goto check_requests_outstanding;
> +
> /* Don't commit yet if this is a non-blocking flush and there
> * are a lot of outstanding writes for this mapping.
> */
> @@ -1934,16 +1937,16 @@ int nfs_write_inode(struct inode *inode, struct writeback_control *wbc)
> flags = 0;
> }
>
> - ret = nfs_commit_inode(inode, flags);
> - if (ret >= 0) {
> - if (wbc->sync_mode == WB_SYNC_NONE) {
> - if (ret < wbc->nr_to_write)
> - wbc->nr_to_write -= ret;
> - else
> - wbc->nr_to_write = 0;
> - }
> - return 0;
> - }
> + ret = __nfs_commit_inode(inode, flags, wbc);
> + if (!ret) {
> + if (flags & FLUSH_SYNC)
> + return 0;
> + } else if (atomic_long_read(&nfsi->commit_info.ncommit))
> + goto out_mark_dirty;
> +
> +check_requests_outstanding:
> + if (!atomic_read(&nfsi->commit_info.rpcs_out))
> + return ret;
> out_mark_dirty:
> __mark_inode_dirty(inode, I_DIRTY_DATASYNC);
> return ret;
> --
> 2.14.3
>
> --
> Trond Myklebust
> Linux NFS client maintainer, PrimaryData
> trond.myklebust@xxxxxxxxxxxxxxx
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
