From patchwork Fri Jun 15 12:39:56 2018 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Brian Foster X-Patchwork-Id: 10466323 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork.web.codeaurora.org (Postfix) with ESMTP id 2D58560384 for ; Fri, 15 Jun 2018 12:40:00 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 2009328D8B for ; Fri, 15 Jun 2018 12:40:00 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id 1469C28D90; Fri, 15 Jun 2018 12:40:00 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-7.9 required=2.0 tests=BAYES_00, MAILING_LIST_MULTI, RCVD_IN_DNSWL_HI autolearn=ham version=3.3.1 Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 7A6CA28D8B for ; Fri, 15 Jun 2018 12:39:59 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755827AbeFOMj6 (ORCPT ); Fri, 15 Jun 2018 08:39:58 -0400 Received: from mx3-rdu2.redhat.com ([66.187.233.73]:55424 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1754760AbeFOMj6 (ORCPT ); Fri, 15 Jun 2018 08:39:58 -0400 Received: from smtp.corp.redhat.com (int-mx04.intmail.prod.int.rdu2.redhat.com [10.11.54.4]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id E07028D76F; Fri, 15 Jun 2018 12:39:57 +0000 (UTC) Received: from bfoster (dhcp-41-2.bos.redhat.com [10.18.41.2]) by smtp.corp.redhat.com (Postfix) with ESMTPS id C05A72026990; Fri, 15 Jun 2018 12:39:57 +0000 (UTC) Date: Fri, 15 Jun 2018 08:39:56 -0400 From: Brian Foster To: Christoph Hellwig Cc: linux-xfs@vger.kernel.org Subject: Re: [PATCH v2 2/2] xfs: use sync buffer I/O for sync delwri queue submission Message-ID: <20180615123956.GD2857@bfoster> References: <20180613110516.65494-1-bfoster@redhat.com> <20180613110516.65494-3-bfoster@redhat.com> <20180615112820.GB3230@infradead.org> <20180615115334.GC2857@bfoster> <20180615121602.GA32099@infradead.org> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20180615121602.GA32099@infradead.org> User-Agent: Mutt/1.10.0 (2018-05-17) X-Scanned-By: MIMEDefang 2.78 on 10.11.54.4 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.2]); Fri, 15 Jun 2018 12:39:57 +0000 (UTC) X-Greylist: inspected by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.2]); Fri, 15 Jun 2018 12:39:57 +0000 (UTC) for IP:'10.11.54.4' DOMAIN:'int-mx04.intmail.prod.int.rdu2.redhat.com' HELO:'smtp.corp.redhat.com' FROM:'bfoster@redhat.com' RCPT:'' Sender: linux-xfs-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-xfs@vger.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP On Fri, Jun 15, 2018 at 05:16:02AM -0700, Christoph Hellwig wrote: > On Fri, Jun 15, 2018 at 07:53:35AM -0400, Brian Foster wrote: > > Not totally sure I follow... do you essentially mean to rename > > xfs_buf_submit_wait() -> xfs_buf_submit() and make the iowait > > conditional on !XBF_ASYNC and absence of some new "sync_nowait" > > parameter to the function? Could you clarify how you envision the > > updated xfs_buf_submit() function signature to look? > > > > If I'm following correctly, that seems fairly reasonable at first > > thought. This is a separate patch though (refactoring the interface vs. > > refactoring the implementation to fix a bug). > > Well. I'd suggest something like the patch below for patch 1: > Ok, codewise I don't have much of a preference, but I don't think it's worth redoing the regression testing and lowmem testing and whatnot just to change how the guts are refactored here. What's the endgame? I came up with the following on top of patch 2. Compile tested only, and I can refold the _common() helper back into the caller and invert the nowait logic or whatnot.. Brian --- 8< --- --- To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html diff --git a/fs/xfs/xfs_buf.c b/fs/xfs/xfs_buf.c index 311ca301b7fd..89d8cedf2828 100644 --- a/fs/xfs/xfs_buf.c +++ b/fs/xfs/xfs_buf.c @@ -757,11 +757,7 @@ _xfs_buf_read( bp->b_flags &= ~(XBF_WRITE | XBF_ASYNC | XBF_READ_AHEAD); bp->b_flags |= flags & (XBF_READ | XBF_ASYNC | XBF_READ_AHEAD); - if (flags & XBF_ASYNC) { - xfs_buf_submit(bp); - return 0; - } - return xfs_buf_submit_wait(bp); + return xfs_buf_submit(bp); } xfs_buf_t * @@ -846,7 +842,7 @@ xfs_buf_read_uncached( bp->b_flags |= XBF_READ; bp->b_ops = ops; - xfs_buf_submit_wait(bp); + xfs_buf_submit(bp); if (bp->b_error) { int error = bp->b_error; xfs_buf_relse(bp); @@ -1249,7 +1245,7 @@ xfs_bwrite( bp->b_flags &= ~(XBF_ASYNC | XBF_READ | _XBF_DELWRI_Q | XBF_WRITE_FAIL | XBF_DONE); - error = xfs_buf_submit_wait(bp); + error = xfs_buf_submit(bp); if (error) { xfs_force_shutdown(bp->b_target->bt_mount, SHUTDOWN_META_IO_ERROR); @@ -1459,7 +1455,7 @@ _xfs_buf_ioapply( * itself. */ static int -__xfs_buf_submit( +__xfs_buf_submit_common( struct xfs_buf *bp) { trace_xfs_buf_submit(bp, _RET_IP_); @@ -1505,32 +1501,6 @@ __xfs_buf_submit( return 0; } -void -xfs_buf_submit( - struct xfs_buf *bp) -{ - int error; - - ASSERT(bp->b_flags & XBF_ASYNC); - - /* - * The caller's reference is released during I/O completion. - * This occurs some time after the last b_io_remaining reference is - * released, so after we drop our Io reference we have to have some - * other reference to ensure the buffer doesn't go away from underneath - * us. Take a direct reference to ensure we have safe access to the - * buffer until we are finished with it. - */ - xfs_buf_hold(bp); - - error = __xfs_buf_submit(bp); - if (error) - xfs_buf_ioend(bp); - - /* Note: it is not safe to reference bp now we've dropped our ref */ - xfs_buf_rele(bp); -} - /* * Wait for I/O completion of a sync buffer and return the I/O error code. */ @@ -1549,30 +1519,33 @@ xfs_buf_iowait( * Synchronous buffer IO submission path, read or write. */ int -xfs_buf_submit_wait( - struct xfs_buf *bp) +__xfs_buf_submit( + struct xfs_buf *bp, + bool sync_nowait) { int error; - ASSERT(!(bp->b_flags & XBF_ASYNC)); - /* - * For synchronous IO, the IO does not inherit the submitters reference - * count, nor the buffer lock. Hence we cannot release the reference we - * are about to take until we've waited for all IO completion to occur, - * including any xfs_buf_ioend_async() work that may be pending. + * Grab a reference so the buffer does not go away underneath us. For + * async buffers, I/O completion drops the callers reference, which + * could occur before submission returns. */ xfs_buf_hold(bp); - error = __xfs_buf_submit(bp); - if (error) + error = __xfs_buf_submit_common(bp); + if (error) { + if (bp->b_flags & XBF_ASYNC) + xfs_buf_ioend(bp); goto out; - error = xfs_buf_iowait(bp); + } + if (!(bp->b_flags & XBF_ASYNC) && !sync_nowait) + error = xfs_buf_iowait(bp); out: /* - * all done now, we can release the hold that keeps the buffer - * referenced for the entire IO. + * Release the hold that keeps the buffer referenced for the entire + * I/O. Note that if the buffer is async, it is not safe to reference + * after this release. */ xfs_buf_rele(bp); return error; @@ -1984,6 +1957,7 @@ xfs_buf_delwri_submit_buffers( LIST_HEAD (submit_list); int pinned = 0; struct blk_plug plug; + bool nowait = false; list_sort(NULL, buffer_list, xfs_buf_cmp); @@ -2025,12 +1999,12 @@ xfs_buf_delwri_submit_buffers( if (wait_list) { bp->b_flags &= ~XBF_ASYNC; list_move_tail(&bp->b_list, wait_list); - __xfs_buf_submit(bp); + nowait = true; } else { bp->b_flags |= XBF_ASYNC; list_del_init(&bp->b_list); - xfs_buf_submit(bp); } + __xfs_buf_submit(bp, nowait); } blk_finish_plug(&plug); diff --git a/fs/xfs/xfs_buf.h b/fs/xfs/xfs_buf.h index d24dbd4dac39..9bae5c201003 100644 --- a/fs/xfs/xfs_buf.h +++ b/fs/xfs/xfs_buf.h @@ -298,8 +298,13 @@ extern void __xfs_buf_ioerror(struct xfs_buf *bp, int error, xfs_failaddr_t failaddr); #define xfs_buf_ioerror(bp, err) __xfs_buf_ioerror((bp), (err), __this_address) extern void xfs_buf_ioerror_alert(struct xfs_buf *, const char *func); -extern void xfs_buf_submit(struct xfs_buf *bp); -extern int xfs_buf_submit_wait(struct xfs_buf *bp); + +extern int __xfs_buf_submit(struct xfs_buf *bp, bool); +static inline int xfs_buf_submit(struct xfs_buf *bp) +{ + return __xfs_buf_submit(bp, false); +} + extern void xfs_buf_iomove(xfs_buf_t *, size_t, size_t, void *, xfs_buf_rw_t); #define xfs_buf_zero(bp, off, len) \ diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c index b181b5f57a19..724a76d87564 100644 --- a/fs/xfs/xfs_log_recover.c +++ b/fs/xfs/xfs_log_recover.c @@ -196,7 +196,7 @@ xlog_bread_noalign( bp->b_io_length = nbblks; bp->b_error = 0; - error = xfs_buf_submit_wait(bp); + error = xfs_buf_submit(bp); if (error && !XFS_FORCED_SHUTDOWN(log->l_mp)) xfs_buf_ioerror_alert(bp, __func__); return error; @@ -5707,7 +5707,7 @@ xlog_do_recover( bp->b_flags |= XBF_READ; bp->b_ops = &xfs_sb_buf_ops; - error = xfs_buf_submit_wait(bp); + error = xfs_buf_submit(bp); if (error) { if (!XFS_FORCED_SHUTDOWN(mp)) { xfs_buf_ioerror_alert(bp, __func__);