[PATCH 0/2] Fix a couple comments

[PATCH 0/2] Fix a couple comments

[cem]

From: Carlos Maiolino <cem@kernel.org>

Carlos Maiolino (2):
  Fix comment on xfs_ail_delete
  XFS: Fix comment on xfs_trans_ail_update_bulk()

 fs/xfs/xfs_trans_ail.c | 36 +++++++++++++++++++-----------------
 1 file changed, 19 insertions(+), 17 deletions(-)

-- 
2.49.0

[PATCH 1/2] Fix comment on xfs_ail_delete

[cem]

From: Carlos Maiolino <cem@kernel.org>

It doesn't return anything.

Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com>
---
 fs/xfs/xfs_trans_ail.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/xfs/xfs_trans_ail.c b/fs/xfs/xfs_trans_ail.c
index 85a649fec6ac..7d327a3e5a73 100644
--- a/fs/xfs/xfs_trans_ail.c
+++ b/fs/xfs/xfs_trans_ail.c
@@ -315,7 +315,7 @@ xfs_ail_splice(
 }
 
 /*
- * Delete the given item from the AIL.  Return a pointer to the item.
+ * Delete the given item from the AIL.
  */
 static void
 xfs_ail_delete(
-- 
2.49.0

[PATCH 2/2] XFS: Fix comment on xfs_trans_ail_update_bulk()

[cem]

From: Carlos Maiolino <cem@kernel.org>

This function doesn't take the AIL lock, but should be called
with AIL lock held. Also (hopefuly) simplify the comment.

Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com>
---
 fs/xfs/xfs_trans_ail.c | 34 ++++++++++++++++++----------------
 1 file changed, 18 insertions(+), 16 deletions(-)

diff --git a/fs/xfs/xfs_trans_ail.c b/fs/xfs/xfs_trans_ail.c
index 7d327a3e5a73..ea092368a5c7 100644
--- a/fs/xfs/xfs_trans_ail.c
+++ b/fs/xfs/xfs_trans_ail.c
@@ -777,26 +777,28 @@ xfs_ail_update_finish(
 }
 
 /*
- * xfs_trans_ail_update - bulk AIL insertion operation.
+ * xfs_trans_ail_update_bulk - bulk AIL insertion operation.
  *
- * @xfs_trans_ail_update takes an array of log items that all need to be
+ * @xfs_trans_ail_update_bulk takes an array of log items that all need to be
  * positioned at the same LSN in the AIL. If an item is not in the AIL, it will
- * be added.  Otherwise, it will be repositioned  by removing it and re-adding
- * it to the AIL. If we move the first item in the AIL, update the log tail to
- * match the new minimum LSN in the AIL.
+ * be added. Otherwise, it will be repositioned by removing it and re-adding
+ * it to the AIL.
  *
- * This function takes the AIL lock once to execute the update operations on
- * all the items in the array, and as such should not be called with the AIL
- * lock held. As a result, once we have the AIL lock, we need to check each log
- * item LSN to confirm it needs to be moved forward in the AIL.
+ * If we move the first item in the AIL, update the log tail to match the new
+ * minimum LSN in the AIL.
  *
- * To optimise the insert operation, we delete all the items from the AIL in
- * the first pass, moving them into a temporary list, then splice the temporary
- * list into the correct position in the AIL. This avoids needing to do an
- * insert operation on every item.
+ * This function should be called with the AIL lock held.
  *
- * This function must be called with the AIL lock held.  The lock is dropped
- * before returning.
+ * To optimise the insert operation, we add all items to a temporary list, then
+ * splice this list into the correct position in the AIL.
+ *
+ * Items that are already in the AIL are first deleted from their current location
+ * before being added to the temporary list.
+ *
+ * This avoids needing to do an insert operation on every item.
+ *
+ * The AIL lock is dropped by xfs_ail_update_finish() before returning to
+ * the caller.
  */
 void
 xfs_trans_ail_update_bulk(
@@ -817,7 +819,7 @@ xfs_trans_ail_update_bulk(
 	for (i = 0; i < nr_items; i++) {
 		struct xfs_log_item *lip = log_items[i];
 		if (test_and_set_bit(XFS_LI_IN_AIL, &lip->li_flags)) {
-			/* check if we really need to move the item */
+			* check if we really need to move the item */
 			if (XFS_LSN_CMP(lsn, lip->li_lsn) <= 0)
 				continue;
 
-- 
2.49.0

Re: [PATCH 1/2] Fix comment on xfs_ail_delete

[Darrick J. Wong]

On Wed, May 07, 2025 at 11:52:30AM +0200, cem@kernel.org wrote:
> From: Carlos Maiolino <cem@kernel.org>
> 
> It doesn't return anything.
> 
> Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com>

Indeed it does not.
Reviewed-by: "Darrick J. Wong" <djwong@kernel.org>

--D

> ---
>  fs/xfs/xfs_trans_ail.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/xfs/xfs_trans_ail.c b/fs/xfs/xfs_trans_ail.c
> index 85a649fec6ac..7d327a3e5a73 100644
> --- a/fs/xfs/xfs_trans_ail.c
> +++ b/fs/xfs/xfs_trans_ail.c
> @@ -315,7 +315,7 @@ xfs_ail_splice(
>  }
>  
>  /*
> - * Delete the given item from the AIL.  Return a pointer to the item.
> + * Delete the given item from the AIL.
>   */
>  static void
>  xfs_ail_delete(
> -- 
> 2.49.0
> 
> 

Re: [PATCH 2/2] XFS: Fix comment on xfs_trans_ail_update_bulk()

[Darrick J. Wong]

On Wed, May 07, 2025 at 11:52:31AM +0200, cem@kernel.org wrote:
> From: Carlos Maiolino <cem@kernel.org>
> 
> This function doesn't take the AIL lock, but should be called
> with AIL lock held. Also (hopefuly) simplify the comment.
> 
> Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com>
> ---
>  fs/xfs/xfs_trans_ail.c | 34 ++++++++++++++++++----------------
>  1 file changed, 18 insertions(+), 16 deletions(-)
> 
> diff --git a/fs/xfs/xfs_trans_ail.c b/fs/xfs/xfs_trans_ail.c
> index 7d327a3e5a73..ea092368a5c7 100644
> --- a/fs/xfs/xfs_trans_ail.c
> +++ b/fs/xfs/xfs_trans_ail.c
> @@ -777,26 +777,28 @@ xfs_ail_update_finish(
>  }
>  
>  /*
> - * xfs_trans_ail_update - bulk AIL insertion operation.
> + * xfs_trans_ail_update_bulk - bulk AIL insertion operation.
>   *
> - * @xfs_trans_ail_update takes an array of log items that all need to be
> + * @xfs_trans_ail_update_bulk takes an array of log items that all need to be
>   * positioned at the same LSN in the AIL. If an item is not in the AIL, it will
> - * be added.  Otherwise, it will be repositioned  by removing it and re-adding
> - * it to the AIL. If we move the first item in the AIL, update the log tail to
> - * match the new minimum LSN in the AIL.
> + * be added. Otherwise, it will be repositioned by removing it and re-adding
> + * it to the AIL.
>   *
> - * This function takes the AIL lock once to execute the update operations on
> - * all the items in the array, and as such should not be called with the AIL
> - * lock held. As a result, once we have the AIL lock, we need to check each log
> - * item LSN to confirm it needs to be moved forward in the AIL.
> + * If we move the first item in the AIL, update the log tail to match the new
> + * minimum LSN in the AIL.
>   *
> - * To optimise the insert operation, we delete all the items from the AIL in
> - * the first pass, moving them into a temporary list, then splice the temporary
> - * list into the correct position in the AIL. This avoids needing to do an
> - * insert operation on every item.
> + * This function should be called with the AIL lock held.
>   *
> - * This function must be called with the AIL lock held.  The lock is dropped
> - * before returning.
> + * To optimise the insert operation, we add all items to a temporary list, then
> + * splice this list into the correct position in the AIL.
> + *
> + * Items that are already in the AIL are first deleted from their current location
> + * before being added to the temporary list.
> + *
> + * This avoids needing to do an insert operation on every item.
> + *
> + * The AIL lock is dropped by xfs_ail_update_finish() before returning to
> + * the caller.

/me thinks this sounds right, but maybe check with dchinner...

>   */
>  void
>  xfs_trans_ail_update_bulk(
> @@ -817,7 +819,7 @@ xfs_trans_ail_update_bulk(
>  	for (i = 0; i < nr_items; i++) {
>  		struct xfs_log_item *lip = log_items[i];
>  		if (test_and_set_bit(XFS_LI_IN_AIL, &lip->li_flags)) {
> -			/* check if we really need to move the item */
> +			* check if we really need to move the item */

                        ^busted comment marker?

--D

>  			if (XFS_LSN_CMP(lsn, lip->li_lsn) <= 0)
>  				continue;
>  
> -- 
> 2.49.0
> 
> 

Re: [PATCH 1/2] Fix comment on xfs_ail_delete

[Christoph Hellwig]

The sibject line should be something like:

xfs: fix comment on xfs_ail_delete

Otherwise looks good:

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

Re: [PATCH 2/2] XFS: Fix comment on xfs_trans_ail_update_bulk()

[Christoph Hellwig]

Lot of spurious capitalization in the subject.

> + * Items that are already in the AIL are first deleted from their current location

Overly long line.

> -			/* check if we really need to move the item */
> +			* check if we really need to move the item */

I think this got messed up when sending, as it won't even compile.

Re: [PATCH 2/2] XFS: Fix comment on xfs_trans_ail_update_bulk()

[kernel test robot]

Hi,

kernel test robot noticed the following build errors:

[auto build test ERROR on xfs-linux/for-next]
[also build test ERROR on linus/master v6.15-rc5 next-20250508]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/cem-kernel-org/Fix-comment-on-xfs_ail_delete/20250507-175334
base:   https://git.kernel.org/pub/scm/fs/xfs/xfs-linux.git for-next
patch link:    https://lore.kernel.org/r/20250507095239.477105-3-cem%40kernel.org
patch subject: [PATCH 2/2] XFS: Fix comment on xfs_trans_ail_update_bulk()
config: x86_64-rhel-9.4 (https://download.01.org/0day-ci/archive/20250508/202505082325.eX1iuPLZ-lkp@intel.com/config)
compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250508/202505082325.eX1iuPLZ-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202505082325.eX1iuPLZ-lkp@intel.com/

All errors (new ones prefixed by >>):

   fs/xfs/xfs_trans_ail.c: In function 'xfs_trans_ail_update_bulk':
>> fs/xfs/xfs_trans_ail.c:822:27: error: 'check' undeclared (first use in this function)
     822 |                         * check if we really need to move the item */
         |                           ^~~~~
   fs/xfs/xfs_trans_ail.c:822:27: note: each undeclared identifier is reported only once for each function it appears in
>> fs/xfs/xfs_trans_ail.c:822:32: error: expected ';' before 'if'
     822 |                         * check if we really need to move the item */
         |                                ^~~
         |                                ;


vim +/check +822 fs/xfs/xfs_trans_ail.c

   778	
   779	/*
   780	 * xfs_trans_ail_update_bulk - bulk AIL insertion operation.
   781	 *
   782	 * @xfs_trans_ail_update_bulk takes an array of log items that all need to be
   783	 * positioned at the same LSN in the AIL. If an item is not in the AIL, it will
   784	 * be added. Otherwise, it will be repositioned by removing it and re-adding
   785	 * it to the AIL.
   786	 *
   787	 * If we move the first item in the AIL, update the log tail to match the new
   788	 * minimum LSN in the AIL.
   789	 *
   790	 * This function should be called with the AIL lock held.
   791	 *
   792	 * To optimise the insert operation, we add all items to a temporary list, then
   793	 * splice this list into the correct position in the AIL.
   794	 *
   795	 * Items that are already in the AIL are first deleted from their current location
   796	 * before being added to the temporary list.
   797	 *
   798	 * This avoids needing to do an insert operation on every item.
   799	 *
   800	 * The AIL lock is dropped by xfs_ail_update_finish() before returning to
   801	 * the caller.
   802	 */
   803	void
   804	xfs_trans_ail_update_bulk(
   805		struct xfs_ail		*ailp,
   806		struct xfs_ail_cursor	*cur,
   807		struct xfs_log_item	**log_items,
   808		int			nr_items,
   809		xfs_lsn_t		lsn) __releases(ailp->ail_lock)
   810	{
   811		struct xfs_log_item	*mlip;
   812		xfs_lsn_t		tail_lsn = 0;
   813		int			i;
   814		LIST_HEAD(tmp);
   815	
   816		ASSERT(nr_items > 0);		/* Not required, but true. */
   817		mlip = xfs_ail_min(ailp);
   818	
   819		for (i = 0; i < nr_items; i++) {
   820			struct xfs_log_item *lip = log_items[i];
   821			if (test_and_set_bit(XFS_LI_IN_AIL, &lip->li_flags)) {
 > 822				* check if we really need to move the item */
   823				if (XFS_LSN_CMP(lsn, lip->li_lsn) <= 0)
   824					continue;
   825	
   826				trace_xfs_ail_move(lip, lip->li_lsn, lsn);
   827				if (mlip == lip && !tail_lsn)
   828					tail_lsn = lip->li_lsn;
   829	
   830				xfs_ail_delete(ailp, lip);
   831			} else {
   832				trace_xfs_ail_insert(lip, 0, lsn);
   833			}
   834			lip->li_lsn = lsn;
   835			list_add_tail(&lip->li_ail, &tmp);
   836		}
   837	
   838		if (!list_empty(&tmp))
   839			xfs_ail_splice(ailp, cur, &tmp, lsn);
   840	
   841		/*
   842		 * If this is the first insert, wake up the push daemon so it can
   843		 * actively scan for items to push. We also need to do a log tail
   844		 * LSN update to ensure that it is correctly tracked by the log, so
   845		 * set the tail_lsn to NULLCOMMITLSN so that xfs_ail_update_finish()
   846		 * will see that the tail lsn has changed and will update the tail
   847		 * appropriately.
   848		 */
   849		if (!mlip) {
   850			wake_up_process(ailp->ail_task);
   851			tail_lsn = NULLCOMMITLSN;
   852		}
   853	
   854		xfs_ail_update_finish(ailp, tail_lsn);
   855	}
   856	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH 1/2] Fix comment on xfs_ail_delete

[Carlos Maiolino]

On Wed, May 07, 2025 at 09:15:40PM -0700, Christoph Hellwig wrote:
> The sibject line should be something like:
> 
> xfs: fix comment on xfs_ail_delete
> 
> Otherwise looks good:
> 
> Reviewed-by: Christoph Hellwig <hch@lst.de>

Will fix that and send with the V2, thanks for the RwB

Re: [PATCH 2/2] XFS: Fix comment on xfs_trans_ail_update_bulk()

[Carlos Maiolino]

On Wed, May 07, 2025 at 09:17:51PM -0700, Christoph Hellwig wrote:
> Lot of spurious capitalization in the subject.
> 
> > + * Items that are already in the AIL are first deleted from their current location
> 
> Overly long line.
> 
> > -			/* check if we really need to move the item */
> > +			* check if we really need to move the item */
> 
> I think this got messed up when sending, as it won't even compile.

Ufff, yeah, I did something when sending it but I can't remember what. It
belongs to a working tree, so it's compiling here. I'll fix the subject and send
a proper version of it.

Thanks hch and darrick.