Re: [PATCH v2] xfs: revise function comment for xfs_trans_ail_delete

[Darrick J. Wong]

On Sat, Aug 31, 2019 at 05:23:43PM +0800, yu kuai wrote:
> Since xfs_trans_ail_delete_bulk no longer exists, revising the comment
> for new function xfs_trans_ail_delete.
> 
> Fix following warning:
> make W=1 fs/xfs/xfs_trans_ail.o
> fs/xfs/xfs_trans_ail.c:793: warning: Function parameter or member 
> 'ailp' not described in 'xfs_trans_ail_delete'
> fs/xfs/xfs_trans_ail.c:793: warning: Function parameter or member
> 'lip' not described in 'xfs_trans_ail_delete'
> fs/xfs/xfs_trans_ail.c:793: warning: Function parameter or member
> 'shutdown_type' not described in 'xfs_trans_ail_delete'
> 
> Fixes:27af1bbf5244("xfs: remove xfs_trans_ail_delete_bulk")
> Signed-off-by: yu kuai <yukuai3@xxxxxxxxxx>
> ---
>  fs/xfs/xfs_trans_ail.c | 27 +++++++++++----------------
>  1 file changed, 11 insertions(+), 16 deletions(-)
> 
> diff --git a/fs/xfs/xfs_trans_ail.c b/fs/xfs/xfs_trans_ail.c
> index 6ccfd75..6c43b66e 100644
> --- a/fs/xfs/xfs_trans_ail.c
> +++ b/fs/xfs/xfs_trans_ail.c
> @@ -765,25 +765,20 @@ xfs_ail_delete_one(
>  }
>  
> -/**
> - * Remove a log items from the AIL
> +/*
> + * xfs_trans_ail_delet - remove a log item from the AIL

Function name misspelled.

> - * @xfs_trans_ail_delete_bulk takes an array of log items that all need to
> - * removed from the AIL. The caller is already holding the AIL lock, and done
> - * all the checks necessary to ensure the items passed in via @log_items are
> - * ready for deletion. This includes checking that the items are in the AIL.
> + * @xfs_trans_ail_delete takes a log item that needs to be removed from the
> + * AIL. The caller is already holding the AIL lock, and done all the checks
> + * necessary to ensure the item passed in via @lip are ready for deletion.
> + * This includes checking that the items are in the AIL.
>   *
> - * For each log item to be removed, unlink it  from the AIL, clear the IN_AIL
> - * flag from the item and reset the item's lsn to 0. If we remove the first
> - * item in the AIL, update the log tail to match the new minimum LSN in the
> - * AIL.
> + * For the log item to be removed, call xfs_ail_delete_one to unlink it
> + * from the AIL, clear the IN_AIL flag from the item and reset the item's
> + * lsn to 0. If we remove the first item in the AIL, update the log tail
> + * to match the new minimum LSN in the AIL.

FWIW, now that this is a 30-line function I don't think it helps at all
to describe what the function does.  We mostly care about things that
might not be immediately obvious from reading the function, such as
preconditions and whatever side effects the function has.

/*
 * Remove a log item from the AIL.
 *
 * The caller must already hold the AIL lock and is responsible for
 * ensuring that the item is in the AIL and ready for deletion.  The log
 * item's LSN will be reset to 0, and if it was the first item in the
 * AIL, the log tail will be updated to match the new minimum LSN in the
 * AIL.  The AIL lock will be dropped before returning.
 */
void
xfs_trans_ail_delete(

--D

>   *
> - * This function will not drop the AIL lock until all items are removed from
> - * the AIL to minimise the amount of lock traffic on the AIL. This does not
> - * greatly increase the AIL hold time, but does significantly reduce the amount
> - * of traffic on the lock, especially during IO completion.
> - *
> - * This function must be called with the AIL lock held.  The lock is dropped
> - * before returning.
> + * This function must be called with the AIL lock held. The lock will be
> + * dropped before returning.
>   */
>  void
>  xfs_trans_ail_delete(
> -- 
> 2.7.4
>