From patchwork Fri Jun 15 12:16:02 2018 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Christoph Hellwig X-Patchwork-Id: 10466301 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 11B9E60384 for ; Fri, 15 Jun 2018 12:16:05 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id EBF5F28D8E for ; Fri, 15 Jun 2018 12:16:04 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id E095928D91; Fri, 15 Jun 2018 12:16:04 +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.8 required=2.0 tests=BAYES_00,DKIM_SIGNED, MAILING_LIST_MULTI, RCVD_IN_DNSWL_HI, T_DKIM_INVALID 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 6336028D8E for ; Fri, 15 Jun 2018 12:16:04 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755816AbeFOMQD (ORCPT ); Fri, 15 Jun 2018 08:16:03 -0400 Received: from bombadil.infradead.org ([198.137.202.133]:45642 "EHLO bombadil.infradead.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755650AbeFOMQD (ORCPT ); Fri, 15 Jun 2018 08:16:03 -0400 DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=infradead.org; s=bombadil.20170209; h=In-Reply-To:Content-Type:MIME-Version :References:Message-ID:Subject:Cc:To:From:Date:Sender:Reply-To: Content-Transfer-Encoding:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Id: List-Help:List-Unsubscribe:List-Subscribe:List-Post:List-Owner:List-Archive; bh=GcxRtFjr+g+nJkXYIh3OqGfMBwlvgDjie6xqS22uuR0=; b=W3orEnjE8+Ev/6JcZivV3y/rY mcowgi3hDwKo7sHLD8flDCMLRtBFR3T0K9fFo0xkYwW+A+De2z9RXOKBenCg9jBKkxAdk8qrTk1MY KvrqQZu19l2Z85GYkaaednsf3A4qWzdH534iQgN6sVC2IPY+fvIlOgY/etRoC5Skdf7wuX4Os9eVr HgsbGz9IcXleURW0RbzB6kIMZJBRfH606R/eNQMFZHAw9djERltu0NBprQO3Egq0qf972+HnJ6ARj 8TAFd3GQLCC1Bn0PSzukCOaA3R6EwjHzFZeCnuMd9vENJAoLZKpWSit0y7R/RZ+nBndM5i796D/1U okObSK9+A==; Received: from hch by bombadil.infradead.org with local (Exim 4.90_1 #2 (Red Hat Linux)) id 1fTne6-0000QR-EN; Fri, 15 Jun 2018 12:16:02 +0000 Date: Fri, 15 Jun 2018 05:16:02 -0700 From: Christoph Hellwig To: Brian Foster Cc: Christoph Hellwig , linux-xfs@vger.kernel.org Subject: Re: [PATCH v2 2/2] xfs: use sync buffer I/O for sync delwri queue submission Message-ID: <20180615121602.GA32099@infradead.org> References: <20180613110516.65494-1-bfoster@redhat.com> <20180613110516.65494-3-bfoster@redhat.com> <20180615112820.GB3230@infradead.org> <20180615115334.GC2857@bfoster> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20180615115334.GC2857@bfoster> User-Agent: Mutt/1.9.2 (2017-12-15) X-SRS-Rewrite: SMTP reverse-path rewritten from by bombadil.infradead.org. See http://www.infradead.org/rpr.html 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 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: --- 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 55661cbdb51b..5504525d345b 100644 --- a/fs/xfs/xfs_buf.c +++ b/fs/xfs/xfs_buf.c @@ -1417,28 +1417,33 @@ _xfs_buf_ioapply( blk_finish_plug(&plug); } -/* - * Asynchronous IO submission path. This transfers the buffer lock ownership and - * the current reference to the IO. It is not safe to reference the buffer after - * a call to this function unless the caller holds an additional reference - * itself. - */ -void -xfs_buf_submit( +static int +xfs_buf_iowait( struct xfs_buf *bp) +{ + trace_xfs_buf_iowait(bp, _RET_IP_); + wait_for_completion(&bp->b_iowait); + trace_xfs_buf_iowait_done(bp, _RET_IP_); + return bp->b_error; +} + +static int +__xfs_buf_submit( + struct xfs_buf *bp, + bool wait) { trace_xfs_buf_submit(bp, _RET_IP_); ASSERT(!(bp->b_flags & _XBF_DELWRI_Q)); - ASSERT(bp->b_flags & XBF_ASYNC); /* on shutdown we stale and complete the buffer immediately */ if (XFS_FORCED_SHUTDOWN(bp->b_target->bt_mount)) { xfs_buf_ioerror(bp, -EIO); bp->b_flags &= ~XBF_DONE; xfs_buf_stale(bp); - xfs_buf_ioend(bp); - return; + if (!wait) + xfs_buf_ioend(bp); + return -EIO; } if (bp->b_flags & XBF_WRITE) @@ -1463,7 +1468,8 @@ xfs_buf_submit( * xfs_buf_ioend too early. */ atomic_set(&bp->b_io_remaining, 1); - xfs_buf_ioacct_inc(bp); + if (bp->b_flags & XBF_ASYNC) + xfs_buf_ioacct_inc(bp); _xfs_buf_ioapply(bp); /* @@ -1472,14 +1478,31 @@ xfs_buf_submit( * that we don't return to the caller with completion still pending. */ if (atomic_dec_and_test(&bp->b_io_remaining) == 1) { - if (bp->b_error) + if (bp->b_error || !(bp->b_flags & XBF_ASYNC)) xfs_buf_ioend(bp); else xfs_buf_ioend_async(bp); } + if (wait) + xfs_buf_iowait(bp); xfs_buf_rele(bp); /* Note: it is not safe to reference bp now we've dropped our ref */ + return 0; +} + +/* + * Asynchronous IO submission path. This transfers the buffer lock ownership and + * the current reference to the IO. It is not safe to reference the buffer after + * a call to this function unless the caller holds an additional reference + * itself. + */ +void +xfs_buf_submit( + struct xfs_buf *bp) +{ + ASSERT(bp->b_flags & XBF_ASYNC); + __xfs_buf_submit(bp, false); } /* @@ -1489,60 +1512,8 @@ int xfs_buf_submit_wait( struct xfs_buf *bp) { - int error; - - trace_xfs_buf_submit_wait(bp, _RET_IP_); - - ASSERT(!(bp->b_flags & (_XBF_DELWRI_Q | XBF_ASYNC))); - - if (XFS_FORCED_SHUTDOWN(bp->b_target->bt_mount)) { - xfs_buf_ioerror(bp, -EIO); - xfs_buf_stale(bp); - bp->b_flags &= ~XBF_DONE; - return -EIO; - } - - if (bp->b_flags & XBF_WRITE) - xfs_buf_wait_unpin(bp); - - /* clear the internal error state to avoid spurious errors */ - bp->b_io_error = 0; - - /* - * 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. - */ - xfs_buf_hold(bp); - - /* - * Set the count to 1 initially, this will stop an I/O completion - * callout which happens before we have started all the I/O from calling - * xfs_buf_ioend too early. - */ - atomic_set(&bp->b_io_remaining, 1); - _xfs_buf_ioapply(bp); - - /* - * make sure we run completion synchronously if it raced with us and is - * already complete. - */ - if (atomic_dec_and_test(&bp->b_io_remaining) == 1) - xfs_buf_ioend(bp); - - /* wait for completion before gathering the error from the buffer */ - trace_xfs_buf_iowait(bp, _RET_IP_); - wait_for_completion(&bp->b_iowait); - trace_xfs_buf_iowait_done(bp, _RET_IP_); - error = bp->b_error; - - /* - * all done now, we can release the hold that keeps the buffer - * referenced for the entire IO. - */ - xfs_buf_rele(bp); - return error; + ASSERT(!(bp->b_flags & XBF_ASYNC)); + return __xfs_buf_submit(bp, true); } void * diff --git a/fs/xfs/xfs_trace.h b/fs/xfs/xfs_trace.h index 8955254b900e..4d3a9adf0ce3 100644 --- a/fs/xfs/xfs_trace.h +++ b/fs/xfs/xfs_trace.h @@ -322,7 +322,6 @@ DEFINE_BUF_EVENT(xfs_buf_hold); DEFINE_BUF_EVENT(xfs_buf_rele); DEFINE_BUF_EVENT(xfs_buf_iodone); DEFINE_BUF_EVENT(xfs_buf_submit); -DEFINE_BUF_EVENT(xfs_buf_submit_wait); DEFINE_BUF_EVENT(xfs_buf_lock); DEFINE_BUF_EVENT(xfs_buf_lock_done); DEFINE_BUF_EVENT(xfs_buf_trylock_fail);