From: Allison Collins <allison.henderson@oracle.com>
To: Brian Foster <bfoster@redhat.com>
Cc: linux-xfs@vger.kernel.org
Subject: Re: [PATCH v3 08/19] xfs: Factor up commit from xfs_attr_try_sf_addname
Date: Fri, 20 Sep 2019 18:25:04 -0700 [thread overview]
Message-ID: <1336049c-33fb-ee2d-53e6-28f27a576e82@oracle.com> (raw)
In-Reply-To: <20190920135037.GF40150@bfoster>
On 9/20/19 6:50 AM, Brian Foster wrote:
> On Thu, Sep 05, 2019 at 03:18:26PM -0700, Allison Collins wrote:
>> New delayed attribute routines cannot handle transactions,
>> so factor this up to the calling function.
>>
>> Signed-off-by: Allison Collins <allison.henderson@oracle.com>
>> ---
>> fs/xfs/libxfs/xfs_attr.c | 15 ++++++++-------
>> 1 file changed, 8 insertions(+), 7 deletions(-)
>>
>> diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
>> index f27e2c6..318c543 100644
>> --- a/fs/xfs/libxfs/xfs_attr.c
>> +++ b/fs/xfs/libxfs/xfs_attr.c
>> @@ -227,7 +227,7 @@ xfs_attr_try_sf_addname(
>> {
>>
>> struct xfs_mount *mp = dp->i_mount;
>> - int error, error2;
>> + int error;
>>
>> error = xfs_attr_shortform_addname(args);
>> if (error == -ENOSPC)
>> @@ -243,9 +243,7 @@ xfs_attr_try_sf_addname(
>> if (mp->m_flags & XFS_MOUNT_WSYNC)
>> xfs_trans_set_sync(args->trans);
>>
>
> Perhaps the above check should stay along with the tx commit code..?
That makes sense, I will move it upwards
>
>> - error2 = xfs_trans_commit(args->trans);
>> - args->trans = NULL;
>> - return error ? error : error2;
>> + return error;
>> }
>>
>> /*
>> @@ -257,7 +255,7 @@ xfs_attr_set_args(
>> {
>> struct xfs_inode *dp = args->dp;
>> struct xfs_buf *leaf_bp = NULL;
>> - int error;
>> + int error, error2 = 0;;
>>
>> /*
>> * If the attribute list is non-existent or a shortform list,
>> @@ -277,8 +275,11 @@ xfs_attr_set_args(
>> * Try to add the attr to the attribute list in the inode.
>> */
>> error = xfs_attr_try_sf_addname(dp, args);
>> - if (error != -ENOSPC)
>> - return error;
>> + if (!error) {
>> + error2 = xfs_trans_commit(args->trans);
>> + args->trans = NULL;
>> + return error ? error : error2;
>
> We've already checked that error == 0 here, so this can be simplified.
> Hmm.. that said, the original code looks like it commits the transaction
> on error != -ENOSPC, which means this slightly changes behavior when
> (error && error != -ENOSPC) is true. So perhaps it is the error check
> that should be fixed up and not the error2 logic..
Yes, I believe this got some attention in the last review. While it is
different logic now, I think we reasoned that committing on say -EIO or
some other such unexpected error didn't make much sense either, so we
cleaned it up a bit. Though you're probably right about the
simplification now with the change. Is there a reason we would want to
commit in the case of unexpected errors?
Allison
>
> Brian
>
>> + }
>>
>> /*
>> * It won't fit in the shortform, transform to a leaf block.
>> --
>> 2.7.4
>>
next prev parent reply other threads:[~2019-09-21 1:26 UTC|newest]
Thread overview: 64+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-09-05 22:18 [PATCH v3 00/19] Delayed Attributes Allison Collins
2019-09-05 22:18 ` [PATCH v3 01/19] xfs: Replace attribute parameters with struct xfs_name Allison Collins
2019-09-18 16:43 ` Brian Foster
2019-09-18 18:09 ` Allison Collins
2019-09-18 18:14 ` Brian Foster
2019-09-18 18:48 ` Allison Collins
2019-09-18 19:06 ` Darrick J. Wong
2019-09-05 22:18 ` [PATCH v3 02/19] xfs: Embed struct xfs_name in xfs_da_args Allison Collins
2019-09-18 16:44 ` Brian Foster
2019-09-18 16:47 ` Christoph Hellwig
2019-09-18 19:55 ` Allison Collins
2019-09-05 22:18 ` [PATCH v3 03/19] xfs: Add xfs_dabuf defines Allison Collins
2019-09-06 3:37 ` Darrick J. Wong
2019-09-05 22:18 ` [PATCH v3 04/19] xfs: Add xfs_has_attr and subroutines Allison Collins
2019-09-19 17:47 ` Brian Foster
2019-09-19 23:51 ` Allison Collins
2019-09-20 12:32 ` Brian Foster
2019-09-05 22:18 ` [PATCH v3 05/19] xfs: Factor out new helper functions xfs_attr_rmtval_set Allison Collins
2019-09-20 13:49 ` Brian Foster
2019-09-21 7:29 ` Allison Collins
2019-09-05 22:18 ` [PATCH v3 06/19] xfs: Factor up trans handling in xfs_attr3_leaf_flipflags Allison Collins
2019-09-20 13:49 ` Brian Foster
2019-09-21 7:00 ` Allison Collins
2019-09-05 22:18 ` [PATCH v3 07/19] xfs: Factor out xfs_attr_leaf_addname helper Allison Collins
2019-09-20 13:49 ` Brian Foster
2019-09-21 7:00 ` Allison Collins
2019-09-05 22:18 ` [PATCH v3 08/19] xfs: Factor up commit from xfs_attr_try_sf_addname Allison Collins
2019-09-20 13:50 ` Brian Foster
2019-09-21 1:25 ` Allison Collins [this message]
2019-09-23 12:04 ` Brian Foster
2019-09-05 22:18 ` [PATCH v3 09/19] xfs: Factor up trans roll from xfs_attr3_leaf_setflag Allison Collins
2019-09-20 13:50 ` Brian Foster
2019-09-05 22:18 ` [PATCH v3 10/19] xfs: Add xfs_attr3_leaf helper functions Allison Collins
2019-09-20 13:50 ` Brian Foster
2019-09-21 1:03 ` Allison Collins
2019-09-05 22:18 ` [PATCH v3 11/19] xfs: Factor out xfs_attr_rmtval_invalidate Allison Collins
2019-09-20 13:51 ` Brian Foster
2019-09-20 22:50 ` Allison Collins
2019-09-05 22:18 ` [PATCH v3 12/19] xfs: Factor up trans roll in xfs_attr3_leaf_clearflag Allison Collins
2019-09-20 13:51 ` Brian Foster
2019-09-20 22:49 ` Allison Collins
2019-09-05 22:18 ` [PATCH v3 13/19] xfs: Add delay context to xfs_da_args Allison Collins
2019-09-20 13:51 ` Brian Foster
2019-09-20 22:48 ` Allison Collins
2019-09-05 22:18 ` [PATCH v3 14/19] xfs: Add delayed attribute routines Allison Collins
2019-09-20 15:28 ` Brian Foster
2019-09-20 19:12 ` Allison Collins
2019-09-23 12:04 ` Brian Foster
2019-09-24 5:53 ` Allison Collins
2019-09-24 10:05 ` Brian Foster
2019-09-25 4:36 ` Darrick J. Wong
2019-09-25 11:53 ` Brian Foster
2019-09-25 20:28 ` Allison Collins
2019-09-05 22:18 ` [PATCH v3 15/19] xfs: Set up infastructure for deferred attribute operations Allison Collins
2019-09-05 22:18 ` [PATCH v3 16/19] xfs: Add xfs_attr_set_deferred and xfs_attr_remove_deferred Allison Collins
2019-09-05 22:18 ` [PATCH v3 17/19] xfs: Add feature bit XFS_SB_FEAT_INCOMPAT_LOG_DELATTR Allison Collins
2019-09-05 22:18 ` [PATCH v3 18/19] xfs: Enable delayed attributes Allison Collins
2019-09-05 22:18 ` [PATCH v3 19/19] xfs_io: Add delayed attributes error tag Allison Collins
2019-09-16 12:27 ` [PATCH v3 00/19] Delayed Attributes Brian Foster
2019-09-16 18:41 ` Allison Collins
2019-09-16 19:23 ` Brian Foster
2019-09-16 20:42 ` Allison Collins
2019-09-17 4:43 ` Darrick J. Wong
2019-09-17 12:17 ` Brian Foster
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=1336049c-33fb-ee2d-53e6-28f27a576e82@oracle.com \
--to=allison.henderson@oracle.com \
--cc=bfoster@redhat.com \
--cc=linux-xfs@vger.kernel.org \
/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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).