| Message ID | 20231031095038.1559309-1-hch@lst.de (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [realtime-rmap,branch] xfs: lock the RT bitmap inode in xfs_efi_item_recover | expand |
Hi Christoph, kernel test robot noticed the following build errors: [auto build test ERROR on v6.6] [also build test ERROR on linus/master next-20231031] [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/Christoph-Hellwig/xfs-lock-the-RT-bitmap-inode-in-xfs_efi_item_recover/20231031-175736 base: v6.6 patch link: https://lore.kernel.org/r/20231031095038.1559309-1-hch%40lst.de patch subject: [PATCH, realtime-rmap branch] xfs: lock the RT bitmap inode in xfs_efi_item_recover config: loongarch-randconfig-002-20231031 (https://download.01.org/0day-ci/archive/20231101/202311010017.ZOItqKn9-lkp@intel.com/config) compiler: loongarch64-linux-gcc (GCC) 13.2.0 reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231101/202311010017.ZOItqKn9-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/202311010017.ZOItqKn9-lkp@intel.com/ All errors (new ones prefixed by >>): fs/xfs/xfs_extfree_item.c: In function 'xfs_efi_item_recover': >> fs/xfs/xfs_extfree_item.c:707:29: error: implicit declaration of function 'xfs_efi_is_realtime'; did you mean 'xfs_has_realtime'? [-Werror=implicit-function-declaration] 707 | if (xfs_efi_is_realtime(&fake)) | ^~~~~~~~~~~~~~~~~~~ | xfs_has_realtime >> fs/xfs/xfs_extfree_item.c:708:33: error: implicit declaration of function 'xfs_rtbitmap_lock' [-Werror=implicit-function-declaration] 708 | xfs_rtbitmap_lock(tp, mp); | ^~~~~~~~~~~~~~~~~ cc1: some warnings being treated as errors vim +707 fs/xfs/xfs_extfree_item.c 653 654 /* 655 * Process an extent free intent item that was recovered from 656 * the log. We need to free the extents that it describes. 657 */ 658 STATIC int 659 xfs_efi_item_recover( 660 struct xfs_log_item *lip, 661 struct list_head *capture_list) 662 { 663 struct xfs_trans_res resv; 664 struct xfs_efi_log_item *efip = EFI_ITEM(lip); 665 struct xfs_mount *mp = lip->li_log->l_mp; 666 struct xfs_efd_log_item *efdp; 667 struct xfs_trans *tp; 668 int i; 669 int error = 0; 670 bool requeue_only = false; 671 672 /* 673 * First check the validity of the extents described by the 674 * EFI. If any are bad, then assume that all are bad and 675 * just toss the EFI. 676 */ 677 for (i = 0; i < efip->efi_format.efi_nextents; i++) { 678 if (!xfs_efi_validate_ext(mp, 679 &efip->efi_format.efi_extents[i])) { 680 XFS_CORRUPTION_ERROR(__func__, XFS_ERRLEVEL_LOW, mp, 681 &efip->efi_format, 682 sizeof(efip->efi_format)); 683 return -EFSCORRUPTED; 684 } 685 } 686 687 resv = xlog_recover_resv(&M_RES(mp)->tr_itruncate); 688 error = xfs_trans_alloc(mp, &resv, 0, 0, 0, &tp); 689 if (error) 690 return error; 691 efdp = xfs_trans_get_efd(tp, efip, efip->efi_format.efi_nextents); 692 693 for (i = 0; i < efip->efi_format.efi_nextents; i++) { 694 struct xfs_extent_free_item fake = { 695 .xefi_owner = XFS_RMAP_OWN_UNKNOWN, 696 .xefi_agresv = XFS_AG_RESV_NONE, 697 }; 698 struct xfs_extent *extp; 699 700 extp = &efip->efi_format.efi_extents[i]; 701 702 fake.xefi_startblock = extp->ext_start; 703 fake.xefi_blockcount = extp->ext_len; 704 705 if (!requeue_only) { 706 xfs_extent_free_get_group(mp, &fake); > 707 if (xfs_efi_is_realtime(&fake)) > 708 xfs_rtbitmap_lock(tp, mp); 709 error = xfs_trans_free_extent(tp, efdp, &fake); 710 xfs_extent_free_put_group(&fake); 711 } 712 713 /* 714 * If we can't free the extent without potentially deadlocking, 715 * requeue the rest of the extents to a new so that they get 716 * run again later with a new transaction context. 717 */ 718 if (error == -EAGAIN || requeue_only) { 719 error = xfs_free_extent_later(tp, fake.xefi_startblock, 720 fake.xefi_blockcount, 721 &XFS_RMAP_OINFO_ANY_OWNER, 722 fake.xefi_agresv); 723 if (!error) { 724 requeue_only = true; 725 continue; 726 } 727 } 728 729 if (error == -EFSCORRUPTED) 730 XFS_CORRUPTION_ERROR(__func__, XFS_ERRLEVEL_LOW, mp, 731 extp, sizeof(*extp)); 732 if (error) 733 goto abort_error; 734 735 } 736 737 return xfs_defer_ops_capture_and_commit(tp, capture_list); 738 739 abort_error: 740 xfs_trans_cancel(tp); 741 return error; 742 } 743
On Tue, Oct 31, 2023 at 10:50:38AM +0100, Christoph Hellwig wrote: > xfs_trans_free_extent expects the rtbitmap and rtsum inodes to be locked. > Ensure that is the case during log recovery as well. > > Fixes: 3ea32f5cc5f9 ("xfs: support logging EFIs for realtime extents") > Signed-off-by: Christoph Hellwig <hch@lst.de> > --- > > I found this when testing the realtime-rmap branch with additional > patches on top. Probably makes sense to just fold it in for the next > rebase of that branch. > > fs/xfs/xfs_extfree_item.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/fs/xfs/xfs_extfree_item.c b/fs/xfs/xfs_extfree_item.c > index 5b3e7dca4e1ba0..070070b6401d66 100644 > --- a/fs/xfs/xfs_extfree_item.c > +++ b/fs/xfs/xfs_extfree_item.c > @@ -768,6 +768,8 @@ xfs_efi_item_recover( > > if (!requeue_only) { > xfs_extent_free_get_group(mp, &fake); > + if (xfs_efi_is_realtime(&fake)) > + xfs_rtbitmap_lock(tp, mp); Curious, I thought that patch "xfs: support logging EFIs for realtime extents" already locked the rt bitmap? resv = xlog_recover_resv(&M_RES(mp)->tr_itruncate); error = xfs_trans_alloc(mp, &resv, 0, 0, 0, &tp); if (error) return error; efdp = xfs_trans_get_efd(tp, efip, efip->efi_format.efi_nextents); /* Lock the rt bitmap if we've any realtime extents to free. */ for (i = 0; i < efip->efi_format.efi_nextents; i++) { struct xfs_extent *extp; extp = &efip->efi_format.efi_extents[i]; if (extp->ext_len & XFS_EFI_EXTLEN_REALTIME_EXT) { xfs_rtbitmap_lock(tp, mp); Here ^^^^^^^^^ break; } } for (i = 0; i < efip->efi_format.efi_nextents; i++) { struct xfs_extent_free_item fake = { .xefi_owner = XFS_RMAP_OWN_UNKNOWN, .xefi_agresv = XFS_AG_RESV_NONE, }; struct xfs_extent *extp; unsigned int len; extp = &efip->efi_format.efi_extents[i]; fake.xefi_startblock = extp->ext_start; len = extp->ext_len; if (len & XFS_EFI_EXTLEN_REALTIME_EXT) { len &= ~XFS_EFI_EXTLEN_REALTIME_EXT; fake.xefi_flags |= XFS_EFI_REALTIME; } fake.xefi_blockcount = len; if (!requeue_only) { xfs_extent_free_get_group(mp, &fake); error = xfs_trans_free_extent(tp, efdp, &fake); xfs_extent_free_put_group(&fake); } So that by the time we get to this part of the loop, the rtbitmap will be locked already? What sort of error message did you get? <confused> --D > error = xfs_trans_free_extent(tp, efdp, &fake); > xfs_extent_free_put_group(&fake); > } > -- > 2.39.2 >
On Tue, Oct 31, 2023 at 09:53:30AM -0700, Darrick J. Wong wrote: > > if (!requeue_only) { > > xfs_extent_free_get_group(mp, &fake); > > + if (xfs_efi_is_realtime(&fake)) > > + xfs_rtbitmap_lock(tp, mp); > > Curious, I thought that patch "xfs: support logging EFIs for realtime > extents" already locked the rt bitmap? > So that by the time we get to this part of the loop, the rtbitmap will > be locked already? Yes. Indeed that problem was caused by my hacked tree. Feel free to discard this message.
diff --git a/fs/xfs/xfs_extfree_item.c b/fs/xfs/xfs_extfree_item.c index 5b3e7dca4e1ba0..070070b6401d66 100644 --- a/fs/xfs/xfs_extfree_item.c +++ b/fs/xfs/xfs_extfree_item.c @@ -768,6 +768,8 @@ xfs_efi_item_recover( if (!requeue_only) { xfs_extent_free_get_group(mp, &fake); + if (xfs_efi_is_realtime(&fake)) + xfs_rtbitmap_lock(tp, mp); error = xfs_trans_free_extent(tp, efdp, &fake); xfs_extent_free_put_group(&fake); }
xfs_trans_free_extent expects the rtbitmap and rtsum inodes to be locked. Ensure that is the case during log recovery as well. Fixes: 3ea32f5cc5f9 ("xfs: support logging EFIs for realtime extents") Signed-off-by: Christoph Hellwig <hch@lst.de> --- I found this when testing the realtime-rmap branch with additional patches on top. Probably makes sense to just fold it in for the next rebase of that branch. fs/xfs/xfs_extfree_item.c | 2 ++ 1 file changed, 2 insertions(+)