From patchwork Thu Apr 25 05:47:47 2019 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: "Darrick J. Wong" X-Patchwork-Id: 10916039 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork-2.web.codeaurora.org (Postfix) with ESMTP id 9F30C13B5 for ; Thu, 25 Apr 2019 05:48:04 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 8805628A2D for ; Thu, 25 Apr 2019 05:48:04 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id 7C11B28BE6; Thu, 25 Apr 2019 05:48: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=-8.0 required=2.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,MAILING_LIST_MULTI,RCVD_IN_DNSWL_HI, UNPARSEABLE_RELAY 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 D502E28A2D for ; Thu, 25 Apr 2019 05:48:03 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S2387428AbfDYFsC (ORCPT ); Thu, 25 Apr 2019 01:48:02 -0400 Received: from userp2120.oracle.com ([156.151.31.85]:51928 "EHLO userp2120.oracle.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S2387415AbfDYFsB (ORCPT ); Thu, 25 Apr 2019 01:48:01 -0400 Received: from pps.filterd (userp2120.oracle.com [127.0.0.1]) by userp2120.oracle.com (8.16.0.27/8.16.0.27) with SMTP id x3P5iWg6171937; Thu, 25 Apr 2019 05:47:51 GMT DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=oracle.com; h=date : from : to : cc : subject : message-id : mime-version : content-type; s=corp-2018-07-02; bh=o4kEd14VtKW6yQLdbfnglLcwQwA55YoN6JByhkpRO2U=; b=CSRluOFsZyvD9xauWTqKS+9VrO8/WJPu0LoWg5N4qYoc7tO4HL220KDpbYXk+MPlWXEF 5aiu1wImFu5RQylkw9zNbGCer7p/H2l3zPoHpKOIwn8UAuu8JLxbFz0dWMmcQTI4Vy0f CAzmBoaG2FJXg8TWTXN9hrYJqVh0URtD0u4qfqGnTLWxhsVOJ2PWzyqmOCidSIhp4xJL Wi7/PG9oYwpSk7NsjQNGJk0dUI0GOTVctEVBBgtDqkKIvP+avqymLhCWQ10uwpKrmtxm uYiY9N47u6gpRUtCTW/65gFe1p5a/krBp8PpqqOGaSGgoNQONcAsmuQD8ezEjFcKLM2A ug== Received: from userp3020.oracle.com (userp3020.oracle.com [156.151.31.79]) by userp2120.oracle.com with ESMTP id 2ryv2qe23b-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK); Thu, 25 Apr 2019 05:47:50 +0000 Received: from pps.filterd (userp3020.oracle.com [127.0.0.1]) by userp3020.oracle.com (8.16.0.27/8.16.0.27) with SMTP id x3P5krdU127445; Thu, 25 Apr 2019 05:47:50 GMT Received: from aserv0121.oracle.com (aserv0121.oracle.com [141.146.126.235]) by userp3020.oracle.com with ESMTP id 2s0dwf7c09-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK); Thu, 25 Apr 2019 05:47:50 +0000 Received: from abhmp0016.oracle.com (abhmp0016.oracle.com [141.146.116.22]) by aserv0121.oracle.com (8.14.4/8.13.8) with ESMTP id x3P5lnQJ003301; Thu, 25 Apr 2019 05:47:49 GMT Received: from localhost (/67.169.218.210) by default (Oracle Beehive Gateway v4.0) with ESMTP ; Wed, 24 Apr 2019 22:47:49 -0700 Date: Wed, 24 Apr 2019 22:47:47 -0700 From: "Darrick J. Wong" To: xfs Cc: Brian Foster Subject: [PATCH 1/2] xfs: always rejoin held resources during defer roll Message-ID: <20190425054747.GG178290@magnolia> MIME-Version: 1.0 Content-Disposition: inline User-Agent: Mutt/1.9.4 (2018-02-28) X-Proofpoint-Virus-Version: vendor=nai engine=5900 definitions=9237 signatures=668685 X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 suspectscore=0 malwarescore=0 phishscore=0 bulkscore=0 spamscore=0 mlxscore=0 mlxlogscore=999 adultscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.0.1-1810050000 definitions=main-1904250039 X-Proofpoint-Virus-Version: vendor=nai engine=5900 definitions=9237 signatures=668685 X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 priorityscore=1501 malwarescore=0 suspectscore=0 phishscore=0 bulkscore=0 spamscore=0 clxscore=1015 lowpriorityscore=0 mlxscore=0 impostorscore=0 mlxlogscore=999 adultscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.0.1-1810050000 definitions=main-1904250039 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 From: Darrick J. Wong During testing of xfs/141 on a V4 filesystem, I observed some inconsistent behavior with regards to resources that are held (i.e. remain locked) across a defer roll. The transaction roll always gives the defer roll function a new transaction, even if committing the old transaction fails. However, the defer roll function only rejoins the held resources if the transaction commit succeedied. This means that callers of defer roll have to figure out whether the held resources are attached to the transaction being passed back. Worse yet, if the defer roll was part of a defer finish call, we have a third possibility: the defer finish could pass back a dirty transaction with dirty held resources and an error code. The only sane way to handle all of these scenarios is to require that the code that held the resource either cancel the transaction before unlocking and releasing the resources, or use functions that detach resources from a transaction properly (e.g. xfs_trans_brelse) if they need to drop the reference before committing or cancelling the transaction. In order to make this so, change the defer roll code to join held resources to the new transaction unconditionally and fix all the bhold callers to release the held buffers correctly. Signed-off-by: Darrick J. Wong --- fs/xfs/libxfs/xfs_attr.c | 31 ++++++++++--------------------- fs/xfs/libxfs/xfs_attr.h | 2 +- fs/xfs/libxfs/xfs_defer.c | 14 +++++++++----- fs/xfs/xfs_dquot.c | 22 +++++++++++----------- 4 files changed, 31 insertions(+), 38 deletions(-) diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c index 2dd9ee2a2e08..ad954138243e 100644 --- a/fs/xfs/libxfs/xfs_attr.c +++ b/fs/xfs/libxfs/xfs_attr.c @@ -224,10 +224,10 @@ xfs_attr_try_sf_addname( */ int xfs_attr_set_args( - struct xfs_da_args *args, - struct xfs_buf **leaf_bp) + struct xfs_da_args *args) { struct xfs_inode *dp = args->dp; + struct xfs_buf *leaf_bp = NULL; int error; /* @@ -255,7 +255,7 @@ xfs_attr_set_args( * It won't fit in the shortform, transform to a leaf block. * GROT: another possible req'mt for a double-split btree op. */ - error = xfs_attr_shortform_to_leaf(args, leaf_bp); + error = xfs_attr_shortform_to_leaf(args, &leaf_bp); if (error) return error; @@ -264,22 +264,15 @@ xfs_attr_set_args( * concurrent AIL push cannot grab the half-baked leaf * buffer and run into problems with the write verifier. */ - xfs_trans_bhold(args->trans, *leaf_bp); + xfs_trans_bhold(args->trans, leaf_bp); error = xfs_defer_finish(&args->trans); - if (error) + if (error) { + xfs_trans_brelse(args->trans, leaf_bp); return error; + } - /* - * Commit the leaf transformation. We'll need another - * (linked) transaction to add the new attribute to the - * leaf. - */ - error = xfs_trans_roll_inode(&args->trans, dp); - if (error) - return error; - xfs_trans_bjoin(args->trans, *leaf_bp); - *leaf_bp = NULL; + xfs_trans_bhold_release(args->trans, leaf_bp); } if (xfs_bmap_one_block(dp, XFS_ATTR_FORK)) @@ -322,7 +315,6 @@ xfs_attr_set( int flags) { struct xfs_mount *mp = dp->i_mount; - struct xfs_buf *leaf_bp = NULL; struct xfs_da_args args; struct xfs_trans_res tres; int rsvd = (flags & ATTR_ROOT) != 0; @@ -381,9 +373,9 @@ xfs_attr_set( goto out_trans_cancel; xfs_trans_ijoin(args.trans, dp, 0); - error = xfs_attr_set_args(&args, &leaf_bp); + error = xfs_attr_set_args(&args); if (error) - goto out_release_leaf; + goto out_trans_cancel; if (!args.trans) { /* shortform attribute has already been committed */ goto out_unlock; @@ -408,9 +400,6 @@ xfs_attr_set( xfs_iunlock(dp, XFS_ILOCK_EXCL); return error; -out_release_leaf: - if (leaf_bp) - xfs_trans_brelse(args.trans, leaf_bp); out_trans_cancel: if (args.trans) xfs_trans_cancel(args.trans); diff --git a/fs/xfs/libxfs/xfs_attr.h b/fs/xfs/libxfs/xfs_attr.h index 2297d8467666..3b0dce06e454 100644 --- a/fs/xfs/libxfs/xfs_attr.h +++ b/fs/xfs/libxfs/xfs_attr.h @@ -140,7 +140,7 @@ int xfs_attr_get(struct xfs_inode *ip, const unsigned char *name, unsigned char *value, int *valuelenp, int flags); int xfs_attr_set(struct xfs_inode *dp, const unsigned char *name, unsigned char *value, int valuelen, int flags); -int xfs_attr_set_args(struct xfs_da_args *args, struct xfs_buf **leaf_bp); +int xfs_attr_set_args(struct xfs_da_args *args); int xfs_attr_remove(struct xfs_inode *dp, const unsigned char *name, int flags); int xfs_attr_remove_args(struct xfs_da_args *args); int xfs_attr_list(struct xfs_inode *dp, char *buffer, int bufsize, diff --git a/fs/xfs/libxfs/xfs_defer.c b/fs/xfs/libxfs/xfs_defer.c index 94f00427de98..1c6bf2105939 100644 --- a/fs/xfs/libxfs/xfs_defer.c +++ b/fs/xfs/libxfs/xfs_defer.c @@ -274,13 +274,15 @@ xfs_defer_trans_roll( trace_xfs_defer_trans_roll(tp, _RET_IP_); - /* Roll the transaction. */ + /* + * Roll the transaction. Rolling always given a new transaction (even + * if committing the old one fails!) to hand back to the caller, so we + * join the held resources to the new transaction so that we always + * return with the held resources joined to @tpp, no matter what + * happened. + */ error = xfs_trans_roll(tpp); tp = *tpp; - if (error) { - trace_xfs_defer_trans_roll_error(tp, error); - return error; - } /* Rejoin the joined inodes. */ for (i = 0; i < ipcount; i++) @@ -292,6 +294,8 @@ xfs_defer_trans_roll( xfs_trans_bhold(tp, bplist[i]); } + if (error) + trace_xfs_defer_trans_roll_error(tp, error); return error; } diff --git a/fs/xfs/xfs_dquot.c b/fs/xfs/xfs_dquot.c index 87e6dd5326d5..7f058120a921 100644 --- a/fs/xfs/xfs_dquot.c +++ b/fs/xfs/xfs_dquot.c @@ -359,9 +359,8 @@ xfs_dquot_disk_alloc( */ xfs_trans_bhold(tp, bp); error = xfs_defer_finish(tpp); - tp = *tpp; if (error) { - xfs_buf_relse(bp); + xfs_trans_brelse(*tpp, bp); return error; } *bpp = bp; @@ -521,7 +520,7 @@ xfs_qm_dqread_alloc( struct xfs_buf **bpp) { struct xfs_trans *tp; - struct xfs_buf *bp; + struct xfs_buf *bp = NULL; int error; error = xfs_trans_alloc(mp, &M_RES(mp)->tr_qm_dqalloc, @@ -529,24 +528,25 @@ xfs_qm_dqread_alloc( if (error) goto err; + /* + * This function returns with the buffer held to the transaction, so + * either we pass it back to our caller still locked or cancel the + * transaction and unlock it manually if we're not passing it back. + */ error = xfs_dquot_disk_alloc(&tp, dqp, &bp); if (error) goto err_cancel; error = xfs_trans_commit(tp); - if (error) { - /* - * Buffer was held to the transaction, so we have to unlock it - * manually here because we're not passing it back. - */ - xfs_buf_relse(bp); - goto err; - } + if (error) + goto err_cancel; *bpp = bp; return 0; err_cancel: xfs_trans_cancel(tp); + if (bp) + xfs_buf_relse(bp); err: return error; } From patchwork Thu Apr 25 05:50:01 2019 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: "Darrick J. Wong" X-Patchwork-Id: 10916055 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork-2.web.codeaurora.org (Postfix) with ESMTP id 4B825161F for ; Thu, 25 Apr 2019 05:52:21 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 3C14928BF8 for ; Thu, 25 Apr 2019 05:52:21 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id 2FCEB28C03; Thu, 25 Apr 2019 05:52:21 +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=-8.0 required=2.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,MAILING_LIST_MULTI,RCVD_IN_DNSWL_HI, UNPARSEABLE_RELAY 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 C8C4C28BF8 for ; Thu, 25 Apr 2019 05:52:20 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1727937AbfDYFwU (ORCPT ); Thu, 25 Apr 2019 01:52:20 -0400 Received: from userp2120.oracle.com ([156.151.31.85]:56428 "EHLO userp2120.oracle.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1727309AbfDYFwU (ORCPT ); Thu, 25 Apr 2019 01:52:20 -0400 Received: from pps.filterd (userp2120.oracle.com [127.0.0.1]) by userp2120.oracle.com (8.16.0.27/8.16.0.27) with SMTP id x3P5mWK4174617; Thu, 25 Apr 2019 05:52:04 GMT DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=oracle.com; h=date : from : to : cc : subject : message-id : references : mime-version : content-type : in-reply-to; s=corp-2018-07-02; bh=5mtm1V5AQqpj5A3ggZGSuJ0VmmihZiJ9LvGxzKaUEQk=; b=i4mgY824gRX49lUk/t/WKb/D1zqcSt/FqQCSnS11+OUKVo36RKy0jP4WIW6Vi3Gm5HjJ gEsXISG9HP0MC22owIaUvOP/d38+UCfLpzNXj89G/GQ2r2xwJdDbNF+aCf5AcQXxy7q3 IG7WfIBbR1EN5VlXWCmQGzsL11ujiLFPcVLhgYFl7ly2JIHy2OuUPp9IDV0rtgjmi8DU CBWEUwhKnzToPv+r+vVAlhqXiLO7Xot/gUa9P7zdiT+KzjlMs+O/Wrv0dzkL4LMD8iaZ nJCUUITg9rvXPkobioe+Vk0F6Gv8aCXvS1fUe5ZFIMpHQiVASHrgZjoRIhYLi0vss5W3 9A== Received: from userp3020.oracle.com (userp3020.oracle.com [156.151.31.79]) by userp2120.oracle.com with ESMTP id 2ryv2qe2ub-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK); Thu, 25 Apr 2019 05:52:04 +0000 Received: from pps.filterd (userp3020.oracle.com [127.0.0.1]) by userp3020.oracle.com (8.16.0.27/8.16.0.27) with SMTP id x3P5mm8K130404; Thu, 25 Apr 2019 05:50:04 GMT Received: from aserv0121.oracle.com (aserv0121.oracle.com [141.146.126.235]) by userp3020.oracle.com with ESMTP id 2s0dwf7cm3-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK); Thu, 25 Apr 2019 05:50:04 +0000 Received: from abhmp0003.oracle.com (abhmp0003.oracle.com [141.146.116.9]) by aserv0121.oracle.com (8.14.4/8.13.8) with ESMTP id x3P5o3Vd004372; Thu, 25 Apr 2019 05:50:03 GMT Received: from localhost (/67.169.218.210) by default (Oracle Beehive Gateway v4.0) with ESMTP ; Wed, 24 Apr 2019 22:50:02 -0700 Date: Wed, 24 Apr 2019 22:50:01 -0700 From: "Darrick J. Wong" To: xfs Cc: Brian Foster Subject: [PATCH 2/2] xfs: fix broken bhold behavior in xrep_roll_ag_trans Message-ID: <20190425055000.GH178290@magnolia> References: <20190425054747.GG178290@magnolia> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20190425054747.GG178290@magnolia> User-Agent: Mutt/1.9.4 (2018-02-28) X-Proofpoint-Virus-Version: vendor=nai engine=5900 definitions=9237 signatures=668685 X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 suspectscore=0 malwarescore=0 phishscore=0 bulkscore=0 spamscore=0 mlxscore=0 mlxlogscore=999 adultscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.0.1-1810050000 definitions=main-1904250040 X-Proofpoint-Virus-Version: vendor=nai engine=5900 definitions=9237 signatures=668685 X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 priorityscore=1501 malwarescore=0 suspectscore=0 phishscore=0 bulkscore=0 spamscore=0 clxscore=1015 lowpriorityscore=0 mlxscore=0 impostorscore=0 mlxlogscore=999 adultscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.0.1-1810050000 definitions=main-1904250040 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 From: Darrick J. Wong In xrep_roll_ag_trans, the transaction roll will always set sc->tp to the new transaction, even if committing the old one fails. A bare transaction roll leaves the buffer(s) locked but not joined to the new transaction, so it's not necessary to release the hold if the roll fails. Remove the incorrect xfs_trans_bhold_release calls. Signed-off-by: Darrick J. Wong Reviewed-by: Brian Foster --- fs/xfs/scrub/repair.c | 25 ++++++++----------------- 1 file changed, 8 insertions(+), 17 deletions(-) diff --git a/fs/xfs/scrub/repair.c b/fs/xfs/scrub/repair.c index 5e7e36cdf3d5..eb358f0f5e0a 100644 --- a/fs/xfs/scrub/repair.c +++ b/fs/xfs/scrub/repair.c @@ -136,10 +136,16 @@ xrep_roll_ag_trans( if (sc->sa.agfl_bp) xfs_trans_bhold(sc->tp, sc->sa.agfl_bp); - /* Roll the transaction. */ + /* + * Roll the transaction. We still own the buffer and the buffer lock + * regardless of whether or not the roll succeeds. If the roll fails, + * the buffers will be released during teardown on our way out of the + * kernel. If it succeeds, we join them to the new transaction and + * move on. + */ error = xfs_trans_roll(&sc->tp); if (error) - goto out_release; + return error; /* Join AG headers to the new transaction. */ if (sc->sa.agi_bp) @@ -150,21 +156,6 @@ xrep_roll_ag_trans( xfs_trans_bjoin(sc->tp, sc->sa.agfl_bp); return 0; - -out_release: - /* - * Rolling failed, so release the hold on the buffers. The - * buffers will be released during teardown on our way out - * of the kernel. - */ - if (sc->sa.agi_bp) - xfs_trans_bhold_release(sc->tp, sc->sa.agi_bp); - if (sc->sa.agf_bp) - xfs_trans_bhold_release(sc->tp, sc->sa.agf_bp); - if (sc->sa.agfl_bp) - xfs_trans_bhold_release(sc->tp, sc->sa.agfl_bp); - - return error; } /*