[02/20] xfs: stop using XFS_LI_ABORTED as a parameter flag

Message ID	20190517073119.30178-3-hch@lst.de (mailing list archive)
State	Superseded
Headers	show Return-Path: <linux-xfs-owner@kernel.org> From: Christoph Hellwig <hch@lst.de> To: linux-xfs@vger.kernel.org Subject: [PATCH 02/20] xfs: stop using XFS_LI_ABORTED as a parameter flag Date: Fri, 17 May 2019 09:31:01 +0200 Message-Id: <20190517073119.30178-3-hch@lst.de> In-Reply-To: <20190517073119.30178-1-hch@lst.de> References: <20190517073119.30178-1-hch@lst.de> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit Sender: linux-xfs-owner@vger.kernel.org Precedence: bulk
Series	[01/20] xfs: fix a trivial comment typo in the xfs_trans_committed_bulk \| expand [01/20] xfs: fix a trivial comment typo in the xfs_trans_committed_bulk [02/20] xfs: stop using XFS_LI_ABORTED as a parameter flag [03/20] xfs: don't require log items to implement optional methods [04/20] xfs: remove the dummy iop_push implementation for inode creation items [05/20] xfs: remove the iop_push implementation for quota off items [06/20] xfs: don't use xfs_trans_free_items in the commit path [07/20] xfs: split iop_unlock [08/20] xfs: add a flag to release log items on commit [09/20] xfs: don't cast inode_log_items to get the log_item [10/20] xfs: remove the xfs_log_item_t typedef [11/20] xfs: use a list_head for iclog callbacks [12/20] xfs: remove a pointless comment duplicated above all xfs_item_ops instances [13/20] xfs: merge xfs_efd_init into xfs_trans_get_efd [14/20] xfs: merge xfs_cud_init into xfs_trans_get_cud [15/20] xfs: merge xfs_rud_init into xfs_trans_get_rud [16/20] xfs: merge xfs_bud_init into xfs_trans_get_bud [17/20] xfs: merge xfs_trans_extfree.c into xfs_extfree_item.c [18/20] xfs: merge xfs_trans_refcount.c into xfs_refcount_item.c [19/20] xfs: merge xfs_trans_rmap.c into xfs_rmap_item.c [20/20] xfs: merge xfs_trans_bmap.c into xfs_bmap_item.c

Christoph Hellwig May 17, 2019, 7:31 a.m. UTC

Just pass a straight bool aborted instead of abusing XFS_LI_ABORTED as a
flag in function parameters.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/xfs_log.c     | 25 +++++++++++--------------
 fs/xfs/xfs_log.h     |  2 +-
 fs/xfs/xfs_log_cil.c |  4 ++--
 3 files changed, 14 insertions(+), 17 deletions(-)

Brian Foster May 17, 2019, 2:04 p.m. UTC | #1

On Fri, May 17, 2019 at 09:31:01AM +0200, Christoph Hellwig wrote:
> Just pass a straight bool aborted instead of abusing XFS_LI_ABORTED as a
> flag in function parameters.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  fs/xfs/xfs_log.c     | 25 +++++++++++--------------
>  fs/xfs/xfs_log.h     |  2 +-
>  fs/xfs/xfs_log_cil.c |  4 ++--
>  3 files changed, 14 insertions(+), 17 deletions(-)
> 
...
> diff --git a/fs/xfs/xfs_log_cil.c b/fs/xfs/xfs_log_cil.c
> index 5e595948bc5a..1b54002d3874 100644
> --- a/fs/xfs/xfs_log_cil.c
> +++ b/fs/xfs/xfs_log_cil.c
> @@ -577,7 +577,7 @@ xlog_discard_busy_extents(
>  static void
>  xlog_cil_committed(
>  	void	*args,
> -	int	abort)
> +	bool	abort)
>  {

Just FYI.. this function passes abort to xfs_trans_committed_bulk(),
which also looks like it could be changed from int to bool. That aside:

Reviewed-by: Brian Foster <bfoster@redhat.com>

>  	struct xfs_cil_ctx	*ctx = args;
>  	struct xfs_mount	*mp = ctx->cil->xc_log->l_mp;
> @@ -864,7 +864,7 @@ xlog_cil_push(
>  out_abort_free_ticket:
>  	xfs_log_ticket_put(tic);
>  out_abort:
> -	xlog_cil_committed(ctx, XFS_LI_ABORTED);
> +	xlog_cil_committed(ctx, true);
>  	return -EIO;
>  }
>  
> -- 
> 2.20.1
>

Eric Sandeen May 17, 2019, 2:10 p.m. UTC | #2

On 5/17/19 2:31 AM, Christoph Hellwig wrote:
> Just pass a straight bool aborted instead of abusing XFS_LI_ABORTED as a
> flag in function parameters.

...

>  out_abort:
> -	xlog_cil_committed(ctx, XFS_LI_ABORTED);
> +	xlog_cil_committed(ctx, true);
>  	return -EIO;

Technically fine but I'm kind of on the fence about changes like this;
doesn't it make the code less readable?  Passing a self-documenting
flag makes code reading a lot easier than seeing "true" and having
to cross-reference what the bool means.  What's your thought on how this
helps?  Is it worth keeping things like this more self-documenting?

Thanks,
-Eric

> Signed-off-by: Christoph Hellwig <hch@lst.de>

Christoph Hellwig May 20, 2019, 6:03 a.m. UTC | #3

On Fri, May 17, 2019 at 10:04:48AM -0400, Brian Foster wrote:
> Just FYI.. this function passes abort to xfs_trans_committed_bulk(),
> which also looks like it could be changed from int to bool. That aside:

True.

Christoph Hellwig May 20, 2019, 6:05 a.m. UTC | #4

On Fri, May 17, 2019 at 09:10:49AM -0500, Eric Sandeen wrote:
> On 5/17/19 2:31 AM, Christoph Hellwig wrote:
> > Just pass a straight bool aborted instead of abusing XFS_LI_ABORTED as a
> > flag in function parameters.
> 
> ...
> 
> >  out_abort:
> > -	xlog_cil_committed(ctx, XFS_LI_ABORTED);
> > +	xlog_cil_committed(ctx, true);
> >  	return -EIO;
> 
> Technically fine but I'm kind of on the fence about changes like this;
> doesn't it make the code less readable?  Passing a self-documenting
> flag makes code reading a lot easier than seeing "true" and having
> to cross-reference what the bool means.  What's your thought on how this
> helps?  Is it worth keeping things like this more self-documenting?

I hate this one because it passes a flag that is used for something
entirely different and then actually interpreted as an int in boolean
context later on.  Switching to a proper bool seems like the simplest
cleanup, but we could also add a different self describing flag if
it bothers you.  But it doesn't really seem like we'd ever grow another
flag here.

Darrick J. Wong May 20, 2019, 10:08 p.m. UTC | #5

On Fri, May 17, 2019 at 09:31:01AM +0200, Christoph Hellwig wrote:
> Just pass a straight bool aborted instead of abusing XFS_LI_ABORTED as a
> flag in function parameters.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  fs/xfs/xfs_log.c     | 25 +++++++++++--------------
>  fs/xfs/xfs_log.h     |  2 +-
>  fs/xfs/xfs_log_cil.c |  4 ++--
>  3 files changed, 14 insertions(+), 17 deletions(-)
> 
> diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
> index 457ced3ee3e1..1eb0938165fc 100644
> --- a/fs/xfs/xfs_log.c
> +++ b/fs/xfs/xfs_log.c
> @@ -54,12 +54,9 @@ xlog_dealloc_log(
>  	struct xlog		*log);
>  
>  /* local state machine functions */
> -STATIC void xlog_state_done_syncing(xlog_in_core_t *iclog, int);
> -STATIC void
> -xlog_state_do_callback(
> -	struct xlog		*log,
> -	int			aborted,
> -	struct xlog_in_core	*iclog);
> +STATIC void xlog_state_done_syncing(
> +	struct xlog_in_core	*iclog,
> +	bool			aborted);

I totally mistook this for a function definition. :/

STATIC void xlog_state_done_syncing(struct xlog_in_core *iclog, bool aborted);

...seems to fit on one line, right?

--D

>  STATIC int
>  xlog_state_get_iclog_space(
>  	struct xlog		*log,
> @@ -1255,7 +1252,7 @@ xlog_iodone(xfs_buf_t *bp)
>  {
>  	struct xlog_in_core	*iclog = bp->b_log_item;
>  	struct xlog		*l = iclog->ic_log;
> -	int			aborted = 0;
> +	bool			aborted = false;
>  
>  	/*
>  	 * Race to shutdown the filesystem if we see an error or the iclog is in
> @@ -1275,9 +1272,9 @@ xlog_iodone(xfs_buf_t *bp)
>  		 * callback routines to let them know that the log-commit
>  		 * didn't succeed.
>  		 */
> -		aborted = XFS_LI_ABORTED;
> +		aborted = true;
>  	} else if (iclog->ic_state & XLOG_STATE_IOERROR) {
> -		aborted = XFS_LI_ABORTED;
> +		aborted = true;
>  	}
>  
>  	/* log I/O is always issued ASYNC */
> @@ -2697,7 +2694,7 @@ xlog_get_lowest_lsn(
>  STATIC void
>  xlog_state_do_callback(
>  	struct xlog		*log,
> -	int			aborted,
> +	bool			aborted,
>  	struct xlog_in_core	*ciclog)
>  {
>  	xlog_in_core_t	   *iclog;
> @@ -2936,10 +2933,10 @@ xlog_state_do_callback(
>   */
>  STATIC void
>  xlog_state_done_syncing(
> -	xlog_in_core_t	*iclog,
> -	int		aborted)
> +	struct xlog_in_core	*iclog,
> +	bool			aborted)
>  {
> -	struct xlog	   *log = iclog->ic_log;
> +	struct xlog		*log = iclog->ic_log;
>  
>  	spin_lock(&log->l_icloglock);
>  
> @@ -4026,7 +4023,7 @@ xfs_log_force_umount(
>  	 * avoid races.
>  	 */
>  	wake_up_all(&log->l_cilp->xc_commit_wait);
> -	xlog_state_do_callback(log, XFS_LI_ABORTED, NULL);
> +	xlog_state_do_callback(log, true, NULL);
>  
>  #ifdef XFSERRORDEBUG
>  	{
> diff --git a/fs/xfs/xfs_log.h b/fs/xfs/xfs_log.h
> index 73a64bf32f6f..4450a2a26a1a 100644
> --- a/fs/xfs/xfs_log.h
> +++ b/fs/xfs/xfs_log.h
> @@ -77,7 +77,7 @@ xlog_copy_iovec(struct xfs_log_vec *lv, struct xfs_log_iovec **vecp,
>   */
>  typedef struct xfs_log_callback {
>  	struct xfs_log_callback	*cb_next;
> -	void			(*cb_func)(void *, int);
> +	void			(*cb_func)(void *, bool);
>  	void			*cb_arg;
>  } xfs_log_callback_t;
>  
> diff --git a/fs/xfs/xfs_log_cil.c b/fs/xfs/xfs_log_cil.c
> index 5e595948bc5a..1b54002d3874 100644
> --- a/fs/xfs/xfs_log_cil.c
> +++ b/fs/xfs/xfs_log_cil.c
> @@ -577,7 +577,7 @@ xlog_discard_busy_extents(
>  static void
>  xlog_cil_committed(
>  	void	*args,
> -	int	abort)
> +	bool	abort)
>  {
>  	struct xfs_cil_ctx	*ctx = args;
>  	struct xfs_mount	*mp = ctx->cil->xc_log->l_mp;
> @@ -864,7 +864,7 @@ xlog_cil_push(
>  out_abort_free_ticket:
>  	xfs_log_ticket_put(tic);
>  out_abort:
> -	xlog_cil_committed(ctx, XFS_LI_ABORTED);
> +	xlog_cil_committed(ctx, true);
>  	return -EIO;
>  }
>  
> -- 
> 2.20.1
>

Christoph Hellwig June 11, 2019, 8:46 a.m. UTC | #6

On Mon, May 20, 2019 at 03:08:44PM -0700, Darrick J. Wong wrote:
> > -xlog_state_do_callback(
> > -	struct xlog		*log,
> > -	int			aborted,
> > -	struct xlog_in_core	*iclog);
> > +STATIC void xlog_state_done_syncing(
> > +	struct xlog_in_core	*iclog,
> > +	bool			aborted);
> 
> I totally mistook this for a function definition. :/
> 
> STATIC void xlog_state_done_syncing(struct xlog_in_core *iclog, bool aborted);
> 
> ...seems to fit on one line, right?

Yes, but this style is used by all the forward declarations in
xfs_log.c.  Eventually we should fix them all, or even better get rid
of most of them.

[02/20] xfs: stop using XFS_LI_ABORTED as a parameter flag

Commit Message

Comments

Patch