* [PATCH v3 1/2] xfs: add lock protection when remove perag from radix tree @ 2023-12-13 3:10 Long Li 2023-12-13 3:10 ` [PATCH v3 2/2] xfs: fix perag leak when growfs fails Long Li 2023-12-13 18:18 ` [PATCH v3 1/2] xfs: add lock protection when remove perag from radix tree Darrick J. Wong 0 siblings, 2 replies; 5+ messages in thread From: Long Li @ 2023-12-13 3:10 UTC (permalink / raw) To: djwong, chandanbabu; +Cc: linux-xfs, yi.zhang, houtao1, leo.lilong, yangerkun Take mp->m_perag_lock for deletions from the perag radix tree in xfs_initialize_perag to prevent racing with tagging operations. Lookups are fine - they are RCU protected so already deal with the tree changing shape underneath the lookup - but tagging operations require the tree to be stable while the tags are propagated back up to the root. Right now there's nothing stopping radix tree tagging from operating while a growfs operation is progress and adding/removing new entries into the radix tree. Hence we can have traversals that require a stable tree occurring at the same time we are removing unused entries from the radix tree which causes the shape of the tree to change. Likely this hasn't caused a problem in the past because we are only doing append addition and removal so the active AG part of the tree is not changing shape, but that doesn't mean it is safe. Just making the radix tree modifications serialise against each other is obviously correct. Signed-off-by: Long Li <leo.lilong@huawei.com> Reviewed-by: Christoph Hellwig <hch@lst.de> --- fs/xfs/libxfs/xfs_ag.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/fs/xfs/libxfs/xfs_ag.c b/fs/xfs/libxfs/xfs_ag.c index f62ff125a50a..c730976fdfc0 100644 --- a/fs/xfs/libxfs/xfs_ag.c +++ b/fs/xfs/libxfs/xfs_ag.c @@ -424,13 +424,17 @@ xfs_initialize_perag( out_remove_pag: xfs_defer_drain_free(&pag->pag_intents_drain); + spin_lock(&mp->m_perag_lock); radix_tree_delete(&mp->m_perag_tree, index); + spin_unlock(&mp->m_perag_lock); out_free_pag: kmem_free(pag); out_unwind_new_pags: /* unwind any prior newly initialized pags */ for (index = first_initialised; index < agcount; index++) { + spin_lock(&mp->m_perag_lock); pag = radix_tree_delete(&mp->m_perag_tree, index); + spin_unlock(&mp->m_perag_lock); if (!pag) break; xfs_buf_hash_destroy(pag); -- 2.31.1 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* [PATCH v3 2/2] xfs: fix perag leak when growfs fails 2023-12-13 3:10 [PATCH v3 1/2] xfs: add lock protection when remove perag from radix tree Long Li @ 2023-12-13 3:10 ` Long Li 2023-12-13 18:21 ` Darrick J. Wong 2023-12-13 18:18 ` [PATCH v3 1/2] xfs: add lock protection when remove perag from radix tree Darrick J. Wong 1 sibling, 1 reply; 5+ messages in thread From: Long Li @ 2023-12-13 3:10 UTC (permalink / raw) To: djwong, chandanbabu; +Cc: linux-xfs, yi.zhang, houtao1, leo.lilong, yangerkun During growfs, if new ag in memory has been initialized, however sb_agcount has not been updated, if an error occurs at this time it will cause perag leaks as follows, these new AGs will not been freed during umount , because of these new AGs are not visible(that is included in mp->m_sb.sb_agcount). unreferenced object 0xffff88810be40200 (size 512): comm "xfs_growfs", pid 857, jiffies 4294909093 hex dump (first 32 bytes): 00 c0 c1 05 81 88 ff ff 04 00 00 00 00 00 00 00 ................ 01 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................ backtrace (crc 381741e2): [<ffffffff8191aef6>] __kmalloc+0x386/0x4f0 [<ffffffff82553e65>] kmem_alloc+0xb5/0x2f0 [<ffffffff8238dac5>] xfs_initialize_perag+0xc5/0x810 [<ffffffff824f679c>] xfs_growfs_data+0x9bc/0xbc0 [<ffffffff8250b90e>] xfs_file_ioctl+0x5fe/0x14d0 [<ffffffff81aa5194>] __x64_sys_ioctl+0x144/0x1c0 [<ffffffff83c3d81f>] do_syscall_64+0x3f/0xe0 [<ffffffff83e00087>] entry_SYSCALL_64_after_hwframe+0x62/0x6a unreferenced object 0xffff88810be40800 (size 512): comm "xfs_growfs", pid 857, jiffies 4294909093 hex dump (first 32 bytes): 20 00 00 00 00 00 00 00 57 ef be dc 00 00 00 00 .......W....... 10 08 e4 0b 81 88 ff ff 10 08 e4 0b 81 88 ff ff ................ backtrace (crc bde50e2d): [<ffffffff8191b43a>] __kmalloc_node+0x3da/0x540 [<ffffffff81814489>] kvmalloc_node+0x99/0x160 [<ffffffff8286acff>] bucket_table_alloc.isra.0+0x5f/0x400 [<ffffffff8286bdc5>] rhashtable_init+0x405/0x760 [<ffffffff8238dda3>] xfs_initialize_perag+0x3a3/0x810 [<ffffffff824f679c>] xfs_growfs_data+0x9bc/0xbc0 [<ffffffff8250b90e>] xfs_file_ioctl+0x5fe/0x14d0 [<ffffffff81aa5194>] __x64_sys_ioctl+0x144/0x1c0 [<ffffffff83c3d81f>] do_syscall_64+0x3f/0xe0 [<ffffffff83e00087>] entry_SYSCALL_64_after_hwframe+0x62/0x6a Factor out xfs_free_unused_perag_range() from xfs_initialize_perag(), used for freeing unused perag within a specified range in error handling, included in the error path of the growfs failure. Signed-off-by: Long Li <leo.lilong@huawei.com> --- v3: - Stop use of typedefs for struct pointers - Remove unnecessary indentation - Factor out xfs_free_unused_perag_range() from xfs_initialize_perag(), not xfs_free_perag(). Compared to the v2 version, now the logic for freeing perag in xfs_free_perag() and xfs_initialize_perag() error handling not quite the same. fs/xfs/libxfs/xfs_ag.c | 36 ++++++++++++++++++++++++++---------- fs/xfs/libxfs/xfs_ag.h | 2 ++ fs/xfs/xfs_fsops.c | 5 ++++- 3 files changed, 32 insertions(+), 11 deletions(-) diff --git a/fs/xfs/libxfs/xfs_ag.c b/fs/xfs/libxfs/xfs_ag.c index c730976fdfc0..39d9525270b7 100644 --- a/fs/xfs/libxfs/xfs_ag.c +++ b/fs/xfs/libxfs/xfs_ag.c @@ -332,6 +332,31 @@ xfs_agino_range( return __xfs_agino_range(mp, xfs_ag_block_count(mp, agno), first, last); } +/* + * Free perag within the specified AG range, it is only used to free unused + * perags under the error handling path. + */ +void +xfs_free_unused_perag_range( + struct xfs_mount *mp, + xfs_agnumber_t agstart, + xfs_agnumber_t agend) +{ + struct xfs_perag *pag; + xfs_agnumber_t index; + + for (index = agstart; index < agend; index++) { + spin_lock(&mp->m_perag_lock); + pag = radix_tree_delete(&mp->m_perag_tree, index); + spin_unlock(&mp->m_perag_lock); + if (!pag) + break; + xfs_buf_hash_destroy(pag); + xfs_defer_drain_free(&pag->pag_intents_drain); + kmem_free(pag); + } +} + int xfs_initialize_perag( struct xfs_mount *mp, @@ -431,16 +456,7 @@ xfs_initialize_perag( kmem_free(pag); out_unwind_new_pags: /* unwind any prior newly initialized pags */ - for (index = first_initialised; index < agcount; index++) { - spin_lock(&mp->m_perag_lock); - pag = radix_tree_delete(&mp->m_perag_tree, index); - spin_unlock(&mp->m_perag_lock); - if (!pag) - break; - xfs_buf_hash_destroy(pag); - xfs_defer_drain_free(&pag->pag_intents_drain); - kmem_free(pag); - } + xfs_free_unused_perag_range(mp, first_initialised, agcount); return error; } diff --git a/fs/xfs/libxfs/xfs_ag.h b/fs/xfs/libxfs/xfs_ag.h index 2e0aef87d633..40d7b6427afb 100644 --- a/fs/xfs/libxfs/xfs_ag.h +++ b/fs/xfs/libxfs/xfs_ag.h @@ -133,6 +133,8 @@ __XFS_AG_OPSTATE(prefers_metadata, PREFERS_METADATA) __XFS_AG_OPSTATE(allows_inodes, ALLOWS_INODES) __XFS_AG_OPSTATE(agfl_needs_reset, AGFL_NEEDS_RESET) +void xfs_free_unused_perag_range(struct xfs_mount *mp, xfs_agnumber_t agstart, + xfs_agnumber_t agend); int xfs_initialize_perag(struct xfs_mount *mp, xfs_agnumber_t agcount, xfs_rfsblock_t dcount, xfs_agnumber_t *maxagi); int xfs_initialize_perag_data(struct xfs_mount *mp, xfs_agnumber_t agno); diff --git a/fs/xfs/xfs_fsops.c b/fs/xfs/xfs_fsops.c index e8759b479516..22c3f1e9008e 100644 --- a/fs/xfs/xfs_fsops.c +++ b/fs/xfs/xfs_fsops.c @@ -153,7 +153,7 @@ xfs_growfs_data_private( error = xfs_trans_alloc(mp, &M_RES(mp)->tr_growdata, -delta, 0, 0, &tp); if (error) - return error; + goto out_free_unused_perag; last_pag = xfs_perag_get(mp, oagcount - 1); if (delta > 0) { @@ -227,6 +227,9 @@ xfs_growfs_data_private( out_trans_cancel: xfs_trans_cancel(tp); +out_free_unused_perag: + if (nagcount > oagcount) + xfs_free_unused_perag_range(mp, oagcount, nagcount); return error; } -- 2.31.1 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH v3 2/2] xfs: fix perag leak when growfs fails 2023-12-13 3:10 ` [PATCH v3 2/2] xfs: fix perag leak when growfs fails Long Li @ 2023-12-13 18:21 ` Darrick J. Wong 2023-12-14 2:13 ` Long Li 0 siblings, 1 reply; 5+ messages in thread From: Darrick J. Wong @ 2023-12-13 18:21 UTC (permalink / raw) To: Long Li; +Cc: chandanbabu, linux-xfs, yi.zhang, houtao1, yangerkun On Wed, Dec 13, 2023 at 11:10:13AM +0800, Long Li wrote: > During growfs, if new ag in memory has been initialized, however > sb_agcount has not been updated, if an error occurs at this time it > will cause perag leaks as follows, these new AGs will not been freed > during umount , because of these new AGs are not visible(that is > included in mp->m_sb.sb_agcount). > > unreferenced object 0xffff88810be40200 (size 512): > comm "xfs_growfs", pid 857, jiffies 4294909093 > hex dump (first 32 bytes): > 00 c0 c1 05 81 88 ff ff 04 00 00 00 00 00 00 00 ................ > 01 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................ > backtrace (crc 381741e2): > [<ffffffff8191aef6>] __kmalloc+0x386/0x4f0 > [<ffffffff82553e65>] kmem_alloc+0xb5/0x2f0 > [<ffffffff8238dac5>] xfs_initialize_perag+0xc5/0x810 > [<ffffffff824f679c>] xfs_growfs_data+0x9bc/0xbc0 > [<ffffffff8250b90e>] xfs_file_ioctl+0x5fe/0x14d0 > [<ffffffff81aa5194>] __x64_sys_ioctl+0x144/0x1c0 > [<ffffffff83c3d81f>] do_syscall_64+0x3f/0xe0 > [<ffffffff83e00087>] entry_SYSCALL_64_after_hwframe+0x62/0x6a > unreferenced object 0xffff88810be40800 (size 512): > comm "xfs_growfs", pid 857, jiffies 4294909093 > hex dump (first 32 bytes): > 20 00 00 00 00 00 00 00 57 ef be dc 00 00 00 00 .......W....... > 10 08 e4 0b 81 88 ff ff 10 08 e4 0b 81 88 ff ff ................ > backtrace (crc bde50e2d): > [<ffffffff8191b43a>] __kmalloc_node+0x3da/0x540 > [<ffffffff81814489>] kvmalloc_node+0x99/0x160 > [<ffffffff8286acff>] bucket_table_alloc.isra.0+0x5f/0x400 > [<ffffffff8286bdc5>] rhashtable_init+0x405/0x760 > [<ffffffff8238dda3>] xfs_initialize_perag+0x3a3/0x810 > [<ffffffff824f679c>] xfs_growfs_data+0x9bc/0xbc0 > [<ffffffff8250b90e>] xfs_file_ioctl+0x5fe/0x14d0 > [<ffffffff81aa5194>] __x64_sys_ioctl+0x144/0x1c0 > [<ffffffff83c3d81f>] do_syscall_64+0x3f/0xe0 > [<ffffffff83e00087>] entry_SYSCALL_64_after_hwframe+0x62/0x6a > > Factor out xfs_free_unused_perag_range() from xfs_initialize_perag(), > used for freeing unused perag within a specified range in error handling, > included in the error path of the growfs failure. > > Signed-off-by: Long Li <leo.lilong@huawei.com> This looks like a fix for a bug; is there a reason you omitted a Fixes: tag? Has this bug been here so long that blame points at the granddaddy commit? Code looks ok to me though. --D > --- > v3: > - Stop use of typedefs for struct pointers > - Remove unnecessary indentation > - Factor out xfs_free_unused_perag_range() from xfs_initialize_perag(), > not xfs_free_perag(). Compared to the v2 version, now the logic for > freeing perag in xfs_free_perag() and xfs_initialize_perag() error > handling not quite the same. > fs/xfs/libxfs/xfs_ag.c | 36 ++++++++++++++++++++++++++---------- > fs/xfs/libxfs/xfs_ag.h | 2 ++ > fs/xfs/xfs_fsops.c | 5 ++++- > 3 files changed, 32 insertions(+), 11 deletions(-) > > diff --git a/fs/xfs/libxfs/xfs_ag.c b/fs/xfs/libxfs/xfs_ag.c > index c730976fdfc0..39d9525270b7 100644 > --- a/fs/xfs/libxfs/xfs_ag.c > +++ b/fs/xfs/libxfs/xfs_ag.c > @@ -332,6 +332,31 @@ xfs_agino_range( > return __xfs_agino_range(mp, xfs_ag_block_count(mp, agno), first, last); > } > > +/* > + * Free perag within the specified AG range, it is only used to free unused > + * perags under the error handling path. > + */ > +void > +xfs_free_unused_perag_range( > + struct xfs_mount *mp, > + xfs_agnumber_t agstart, > + xfs_agnumber_t agend) > +{ > + struct xfs_perag *pag; > + xfs_agnumber_t index; > + > + for (index = agstart; index < agend; index++) { > + spin_lock(&mp->m_perag_lock); > + pag = radix_tree_delete(&mp->m_perag_tree, index); > + spin_unlock(&mp->m_perag_lock); > + if (!pag) > + break; > + xfs_buf_hash_destroy(pag); > + xfs_defer_drain_free(&pag->pag_intents_drain); > + kmem_free(pag); > + } > +} > + > int > xfs_initialize_perag( > struct xfs_mount *mp, > @@ -431,16 +456,7 @@ xfs_initialize_perag( > kmem_free(pag); > out_unwind_new_pags: > /* unwind any prior newly initialized pags */ > - for (index = first_initialised; index < agcount; index++) { > - spin_lock(&mp->m_perag_lock); > - pag = radix_tree_delete(&mp->m_perag_tree, index); > - spin_unlock(&mp->m_perag_lock); > - if (!pag) > - break; > - xfs_buf_hash_destroy(pag); > - xfs_defer_drain_free(&pag->pag_intents_drain); > - kmem_free(pag); > - } > + xfs_free_unused_perag_range(mp, first_initialised, agcount); > return error; > } > > diff --git a/fs/xfs/libxfs/xfs_ag.h b/fs/xfs/libxfs/xfs_ag.h > index 2e0aef87d633..40d7b6427afb 100644 > --- a/fs/xfs/libxfs/xfs_ag.h > +++ b/fs/xfs/libxfs/xfs_ag.h > @@ -133,6 +133,8 @@ __XFS_AG_OPSTATE(prefers_metadata, PREFERS_METADATA) > __XFS_AG_OPSTATE(allows_inodes, ALLOWS_INODES) > __XFS_AG_OPSTATE(agfl_needs_reset, AGFL_NEEDS_RESET) > > +void xfs_free_unused_perag_range(struct xfs_mount *mp, xfs_agnumber_t agstart, > + xfs_agnumber_t agend); > int xfs_initialize_perag(struct xfs_mount *mp, xfs_agnumber_t agcount, > xfs_rfsblock_t dcount, xfs_agnumber_t *maxagi); > int xfs_initialize_perag_data(struct xfs_mount *mp, xfs_agnumber_t agno); > diff --git a/fs/xfs/xfs_fsops.c b/fs/xfs/xfs_fsops.c > index e8759b479516..22c3f1e9008e 100644 > --- a/fs/xfs/xfs_fsops.c > +++ b/fs/xfs/xfs_fsops.c > @@ -153,7 +153,7 @@ xfs_growfs_data_private( > error = xfs_trans_alloc(mp, &M_RES(mp)->tr_growdata, -delta, 0, > 0, &tp); > if (error) > - return error; > + goto out_free_unused_perag; > > last_pag = xfs_perag_get(mp, oagcount - 1); > if (delta > 0) { > @@ -227,6 +227,9 @@ xfs_growfs_data_private( > > out_trans_cancel: > xfs_trans_cancel(tp); > +out_free_unused_perag: > + if (nagcount > oagcount) > + xfs_free_unused_perag_range(mp, oagcount, nagcount); > return error; > } > > -- > 2.31.1 > > ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v3 2/2] xfs: fix perag leak when growfs fails 2023-12-13 18:21 ` Darrick J. Wong @ 2023-12-14 2:13 ` Long Li 0 siblings, 0 replies; 5+ messages in thread From: Long Li @ 2023-12-14 2:13 UTC (permalink / raw) To: Darrick J. Wong; +Cc: chandanbabu, linux-xfs, yi.zhang, houtao1, yangerkun On Wed, Dec 13, 2023 at 10:21:12AM -0800, Darrick J. Wong wrote: > On Wed, Dec 13, 2023 at 11:10:13AM +0800, Long Li wrote: > > During growfs, if new ag in memory has been initialized, however > > sb_agcount has not been updated, if an error occurs at this time it > > will cause perag leaks as follows, these new AGs will not been freed > > during umount , because of these new AGs are not visible(that is > > included in mp->m_sb.sb_agcount). > > > > unreferenced object 0xffff88810be40200 (size 512): > > comm "xfs_growfs", pid 857, jiffies 4294909093 > > hex dump (first 32 bytes): > > 00 c0 c1 05 81 88 ff ff 04 00 00 00 00 00 00 00 ................ > > 01 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................ > > backtrace (crc 381741e2): > > [<ffffffff8191aef6>] __kmalloc+0x386/0x4f0 > > [<ffffffff82553e65>] kmem_alloc+0xb5/0x2f0 > > [<ffffffff8238dac5>] xfs_initialize_perag+0xc5/0x810 > > [<ffffffff824f679c>] xfs_growfs_data+0x9bc/0xbc0 > > [<ffffffff8250b90e>] xfs_file_ioctl+0x5fe/0x14d0 > > [<ffffffff81aa5194>] __x64_sys_ioctl+0x144/0x1c0 > > [<ffffffff83c3d81f>] do_syscall_64+0x3f/0xe0 > > [<ffffffff83e00087>] entry_SYSCALL_64_after_hwframe+0x62/0x6a > > unreferenced object 0xffff88810be40800 (size 512): > > comm "xfs_growfs", pid 857, jiffies 4294909093 > > hex dump (first 32 bytes): > > 20 00 00 00 00 00 00 00 57 ef be dc 00 00 00 00 .......W....... > > 10 08 e4 0b 81 88 ff ff 10 08 e4 0b 81 88 ff ff ................ > > backtrace (crc bde50e2d): > > [<ffffffff8191b43a>] __kmalloc_node+0x3da/0x540 > > [<ffffffff81814489>] kvmalloc_node+0x99/0x160 > > [<ffffffff8286acff>] bucket_table_alloc.isra.0+0x5f/0x400 > > [<ffffffff8286bdc5>] rhashtable_init+0x405/0x760 > > [<ffffffff8238dda3>] xfs_initialize_perag+0x3a3/0x810 > > [<ffffffff824f679c>] xfs_growfs_data+0x9bc/0xbc0 > > [<ffffffff8250b90e>] xfs_file_ioctl+0x5fe/0x14d0 > > [<ffffffff81aa5194>] __x64_sys_ioctl+0x144/0x1c0 > > [<ffffffff83c3d81f>] do_syscall_64+0x3f/0xe0 > > [<ffffffff83e00087>] entry_SYSCALL_64_after_hwframe+0x62/0x6a > > > > Factor out xfs_free_unused_perag_range() from xfs_initialize_perag(), > > used for freeing unused perag within a specified range in error handling, > > included in the error path of the growfs failure. > > > > Signed-off-by: Long Li <leo.lilong@huawei.com> > > This looks like a fix for a bug; is there a reason you omitted a Fixes: > tag? Has this bug been here so long that blame points at the > granddaddy commit? Yes, this bug has been around for a long time, and I've looked up the submission history again. Long times ago, it used an array for the per-ag structures, per-ag array can be freed in xfs_free_perag(). After commit 1c1c6ebcf528 ("xfs: Replace per-ag array with a radix tree") was added, only visible AGs will be freed, so it need to add the following fix tag. Fixes: 1c1c6ebcf528 ("xfs: Replace per-ag array with a radix tree") Thanks. > > Code looks ok to me though. > > --D > > > --- > > v3: > > - Stop use of typedefs for struct pointers > > - Remove unnecessary indentation > > - Factor out xfs_free_unused_perag_range() from xfs_initialize_perag(), > > not xfs_free_perag(). Compared to the v2 version, now the logic for > > freeing perag in xfs_free_perag() and xfs_initialize_perag() error > > handling not quite the same. > > fs/xfs/libxfs/xfs_ag.c | 36 ++++++++++++++++++++++++++---------- > > fs/xfs/libxfs/xfs_ag.h | 2 ++ > > fs/xfs/xfs_fsops.c | 5 ++++- > > 3 files changed, 32 insertions(+), 11 deletions(-) > > > > diff --git a/fs/xfs/libxfs/xfs_ag.c b/fs/xfs/libxfs/xfs_ag.c > > index c730976fdfc0..39d9525270b7 100644 > > --- a/fs/xfs/libxfs/xfs_ag.c > > +++ b/fs/xfs/libxfs/xfs_ag.c > > @@ -332,6 +332,31 @@ xfs_agino_range( > > return __xfs_agino_range(mp, xfs_ag_block_count(mp, agno), first, last); > > } > > > > +/* > > + * Free perag within the specified AG range, it is only used to free unused > > + * perags under the error handling path. > > + */ > > +void > > +xfs_free_unused_perag_range( > > + struct xfs_mount *mp, > > + xfs_agnumber_t agstart, > > + xfs_agnumber_t agend) > > +{ > > + struct xfs_perag *pag; > > + xfs_agnumber_t index; > > + > > + for (index = agstart; index < agend; index++) { > > + spin_lock(&mp->m_perag_lock); > > + pag = radix_tree_delete(&mp->m_perag_tree, index); > > + spin_unlock(&mp->m_perag_lock); > > + if (!pag) > > + break; > > + xfs_buf_hash_destroy(pag); > > + xfs_defer_drain_free(&pag->pag_intents_drain); > > + kmem_free(pag); > > + } > > +} > > + > > int > > xfs_initialize_perag( > > struct xfs_mount *mp, > > @@ -431,16 +456,7 @@ xfs_initialize_perag( > > kmem_free(pag); > > out_unwind_new_pags: > > /* unwind any prior newly initialized pags */ > > - for (index = first_initialised; index < agcount; index++) { > > - spin_lock(&mp->m_perag_lock); > > - pag = radix_tree_delete(&mp->m_perag_tree, index); > > - spin_unlock(&mp->m_perag_lock); > > - if (!pag) > > - break; > > - xfs_buf_hash_destroy(pag); > > - xfs_defer_drain_free(&pag->pag_intents_drain); > > - kmem_free(pag); > > - } > > + xfs_free_unused_perag_range(mp, first_initialised, agcount); > > return error; > > } > > > > diff --git a/fs/xfs/libxfs/xfs_ag.h b/fs/xfs/libxfs/xfs_ag.h > > index 2e0aef87d633..40d7b6427afb 100644 > > --- a/fs/xfs/libxfs/xfs_ag.h > > +++ b/fs/xfs/libxfs/xfs_ag.h > > @@ -133,6 +133,8 @@ __XFS_AG_OPSTATE(prefers_metadata, PREFERS_METADATA) > > __XFS_AG_OPSTATE(allows_inodes, ALLOWS_INODES) > > __XFS_AG_OPSTATE(agfl_needs_reset, AGFL_NEEDS_RESET) > > > > +void xfs_free_unused_perag_range(struct xfs_mount *mp, xfs_agnumber_t agstart, > > + xfs_agnumber_t agend); > > int xfs_initialize_perag(struct xfs_mount *mp, xfs_agnumber_t agcount, > > xfs_rfsblock_t dcount, xfs_agnumber_t *maxagi); > > int xfs_initialize_perag_data(struct xfs_mount *mp, xfs_agnumber_t agno); > > diff --git a/fs/xfs/xfs_fsops.c b/fs/xfs/xfs_fsops.c > > index e8759b479516..22c3f1e9008e 100644 > > --- a/fs/xfs/xfs_fsops.c > > +++ b/fs/xfs/xfs_fsops.c > > @@ -153,7 +153,7 @@ xfs_growfs_data_private( > > error = xfs_trans_alloc(mp, &M_RES(mp)->tr_growdata, -delta, 0, > > 0, &tp); > > if (error) > > - return error; > > + goto out_free_unused_perag; > > > > last_pag = xfs_perag_get(mp, oagcount - 1); > > if (delta > 0) { > > @@ -227,6 +227,9 @@ xfs_growfs_data_private( > > > > out_trans_cancel: > > xfs_trans_cancel(tp); > > +out_free_unused_perag: > > + if (nagcount > oagcount) > > + xfs_free_unused_perag_range(mp, oagcount, nagcount); > > return error; > > } > > > > -- > > 2.31.1 > > > > > ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v3 1/2] xfs: add lock protection when remove perag from radix tree 2023-12-13 3:10 [PATCH v3 1/2] xfs: add lock protection when remove perag from radix tree Long Li 2023-12-13 3:10 ` [PATCH v3 2/2] xfs: fix perag leak when growfs fails Long Li @ 2023-12-13 18:18 ` Darrick J. Wong 1 sibling, 0 replies; 5+ messages in thread From: Darrick J. Wong @ 2023-12-13 18:18 UTC (permalink / raw) To: Long Li; +Cc: chandanbabu, linux-xfs, yi.zhang, houtao1, yangerkun On Wed, Dec 13, 2023 at 11:10:12AM +0800, Long Li wrote: > Take mp->m_perag_lock for deletions from the perag radix tree in > xfs_initialize_perag to prevent racing with tagging operations. > Lookups are fine - they are RCU protected so already deal with the > tree changing shape underneath the lookup - but tagging operations > require the tree to be stable while the tags are propagated back up > to the root. > > Right now there's nothing stopping radix tree tagging from operating > while a growfs operation is progress and adding/removing new entries > into the radix tree. > > Hence we can have traversals that require a stable tree occurring at > the same time we are removing unused entries from the radix tree which > causes the shape of the tree to change. > > Likely this hasn't caused a problem in the past because we are only > doing append addition and removal so the active AG part of the tree > is not changing shape, but that doesn't mean it is safe. Just making > the radix tree modifications serialise against each other is obviously > correct. > > Signed-off-by: Long Li <leo.lilong@huawei.com> > Reviewed-by: Christoph Hellwig <hch@lst.de> Looks correct to me, and I didn't find any other suspicious accesses of m_perag_tree so Reviewed-by: Darrick J. Wong <djwong@kernel.org> --D > --- > fs/xfs/libxfs/xfs_ag.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/fs/xfs/libxfs/xfs_ag.c b/fs/xfs/libxfs/xfs_ag.c > index f62ff125a50a..c730976fdfc0 100644 > --- a/fs/xfs/libxfs/xfs_ag.c > +++ b/fs/xfs/libxfs/xfs_ag.c > @@ -424,13 +424,17 @@ xfs_initialize_perag( > > out_remove_pag: > xfs_defer_drain_free(&pag->pag_intents_drain); > + spin_lock(&mp->m_perag_lock); > radix_tree_delete(&mp->m_perag_tree, index); > + spin_unlock(&mp->m_perag_lock); > out_free_pag: > kmem_free(pag); > out_unwind_new_pags: > /* unwind any prior newly initialized pags */ > for (index = first_initialised; index < agcount; index++) { > + spin_lock(&mp->m_perag_lock); > pag = radix_tree_delete(&mp->m_perag_tree, index); > + spin_unlock(&mp->m_perag_lock); > if (!pag) > break; > xfs_buf_hash_destroy(pag); > -- > 2.31.1 > > ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2023-12-14 2:09 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2023-12-13 3:10 [PATCH v3 1/2] xfs: add lock protection when remove perag from radix tree Long Li 2023-12-13 3:10 ` [PATCH v3 2/2] xfs: fix perag leak when growfs fails Long Li 2023-12-13 18:21 ` Darrick J. Wong 2023-12-14 2:13 ` Long Li 2023-12-13 18:18 ` [PATCH v3 1/2] xfs: add lock protection when remove perag from radix tree Darrick J. Wong
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox