* Re: [PATCH 2/2] XFS: Fix comment on xfs_trans_ail_update_bulk()
2025-05-07 9:52 ` [PATCH 2/2] XFS: Fix comment on xfs_trans_ail_update_bulk() cem
@ 2025-05-07 21:07 ` Darrick J. Wong
2025-05-08 4:17 ` Christoph Hellwig
2025-05-08 15:32 ` kernel test robot
2 siblings, 0 replies; 10+ messages in thread
From: Darrick J. Wong @ 2025-05-07 21:07 UTC (permalink / raw)
To: cem; +Cc: linux-xfs
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
>
>
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH 2/2] XFS: Fix comment on xfs_trans_ail_update_bulk()
2025-05-07 9:52 ` [PATCH 2/2] XFS: Fix comment on xfs_trans_ail_update_bulk() cem
2025-05-07 21:07 ` Darrick J. Wong
@ 2025-05-08 4:17 ` Christoph Hellwig
2025-05-09 7:30 ` Carlos Maiolino
2025-05-08 15:32 ` kernel test robot
2 siblings, 1 reply; 10+ messages in thread
From: Christoph Hellwig @ 2025-05-08 4:17 UTC (permalink / raw)
To: cem; +Cc: linux-xfs
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.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] XFS: Fix comment on xfs_trans_ail_update_bulk()
2025-05-08 4:17 ` Christoph Hellwig
@ 2025-05-09 7:30 ` Carlos Maiolino
0 siblings, 0 replies; 10+ messages in thread
From: Carlos Maiolino @ 2025-05-09 7:30 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: linux-xfs
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.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] XFS: Fix comment on xfs_trans_ail_update_bulk()
2025-05-07 9:52 ` [PATCH 2/2] XFS: Fix comment on xfs_trans_ail_update_bulk() cem
2025-05-07 21:07 ` Darrick J. Wong
2025-05-08 4:17 ` Christoph Hellwig
@ 2025-05-08 15:32 ` kernel test robot
2 siblings, 0 replies; 10+ messages in thread
From: kernel test robot @ 2025-05-08 15:32 UTC (permalink / raw)
To: cem, linux-xfs; +Cc: oe-kbuild-all
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
^ permalink raw reply [flat|nested] 10+ messages in thread