From: Christoph Hellwig <hch@lst.de>
Call the provided free_func when xfs_mru_cache_insert as that's what
the callers need to do anyway.
Signed-off-by: Christoph Hellwig <hch@lst.de>
Signed-off-by: Hans Holmberg <hans.holmberg@wdc.com>
---
fs/xfs/xfs_filestream.c | 15 ++++-----------
fs/xfs/xfs_mru_cache.c | 15 ++++++++++++---
2 files changed, 16 insertions(+), 14 deletions(-)
diff --git a/fs/xfs/xfs_filestream.c b/fs/xfs/xfs_filestream.c
index a961aa420c48..044918fbae06 100644
--- a/fs/xfs/xfs_filestream.c
+++ b/fs/xfs/xfs_filestream.c
@@ -304,11 +304,9 @@ xfs_filestream_create_association(
* for us, so all we need to do here is take another active reference to
* the perag for the cached association.
*
- * If we fail to store the association, we need to drop the fstrms
- * counter as well as drop the perag reference we take here for the
- * item. We do not need to return an error for this failure - as long as
- * we return a referenced AG, the allocation can still go ahead just
- * fine.
+ * If we fail to store the association, we do not need to return an
+ * error for this failure - as long as we return a referenced AG, the
+ * allocation can still go ahead just fine.
*/
item = kmalloc(sizeof(*item), GFP_KERNEL | __GFP_RETRY_MAYFAIL);
if (!item)
@@ -316,14 +314,9 @@ xfs_filestream_create_association(
atomic_inc(&pag_group(args->pag)->xg_active_ref);
item->pag = args->pag;
- error = xfs_mru_cache_insert(mp->m_filestream, pino, &item->mru);
- if (error)
- goto out_free_item;
+ xfs_mru_cache_insert(mp->m_filestream, pino, &item->mru);
return 0;
-out_free_item:
- xfs_perag_rele(item->pag);
- kfree(item);
out_put_fstrms:
atomic_dec(&args->pag->pagf_fstrms);
return 0;
diff --git a/fs/xfs/xfs_mru_cache.c b/fs/xfs/xfs_mru_cache.c
index d0f5b403bdbe..08443ceec329 100644
--- a/fs/xfs/xfs_mru_cache.c
+++ b/fs/xfs/xfs_mru_cache.c
@@ -414,6 +414,8 @@ xfs_mru_cache_destroy(
* To insert an element, call xfs_mru_cache_insert() with the data store, the
* element's key and the client data pointer. This function returns 0 on
* success or ENOMEM if memory for the data element couldn't be allocated.
+ *
+ * The passed in elem is freed through the per-cache free_func on failure.
*/
int
xfs_mru_cache_insert(
@@ -421,14 +423,15 @@ xfs_mru_cache_insert(
unsigned long key,
struct xfs_mru_cache_elem *elem)
{
- int error;
+ int error = -EINVAL;
ASSERT(mru && mru->lists);
if (!mru || !mru->lists)
- return -EINVAL;
+ goto out_free;
+ error = -ENOMEM;
if (radix_tree_preload(GFP_KERNEL))
- return -ENOMEM;
+ goto out_free;
INIT_LIST_HEAD(&elem->list_node);
elem->key = key;
@@ -440,6 +443,12 @@ xfs_mru_cache_insert(
_xfs_mru_cache_list_insert(mru, elem);
spin_unlock(&mru->lock);
+ if (error)
+ goto out_free;
+ return 0;
+
+out_free:
+ mru->free_func(mru->data, elem);
return error;
}
--
2.34.1On Wed, Apr 30, 2025 at 08:41:21AM +0000, Hans Holmberg wrote:
> From: Christoph Hellwig <hch@lst.de>
>
> Call the provided free_func when xfs_mru_cache_insert as that's what
> the callers need to do anyway.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> Signed-off-by: Hans Holmberg <hans.holmberg@wdc.com>
> ---
> fs/xfs/xfs_filestream.c | 15 ++++-----------
> fs/xfs/xfs_mru_cache.c | 15 ++++++++++++---
> 2 files changed, 16 insertions(+), 14 deletions(-)
>
> diff --git a/fs/xfs/xfs_filestream.c b/fs/xfs/xfs_filestream.c
> index a961aa420c48..044918fbae06 100644
> --- a/fs/xfs/xfs_filestream.c
> +++ b/fs/xfs/xfs_filestream.c
> @@ -304,11 +304,9 @@ xfs_filestream_create_association(
> * for us, so all we need to do here is take another active reference to
> * the perag for the cached association.
> *
> - * If we fail to store the association, we need to drop the fstrms
> - * counter as well as drop the perag reference we take here for the
> - * item. We do not need to return an error for this failure - as long as
> - * we return a referenced AG, the allocation can still go ahead just
> - * fine.
> + * If we fail to store the association, we do not need to return an
> + * error for this failure - as long as we return a referenced AG, the
> + * allocation can still go ahead just fine.
> */
> item = kmalloc(sizeof(*item), GFP_KERNEL | __GFP_RETRY_MAYFAIL);
> if (!item)
> @@ -316,14 +314,9 @@ xfs_filestream_create_association(
>
> atomic_inc(&pag_group(args->pag)->xg_active_ref);
> item->pag = args->pag;
> - error = xfs_mru_cache_insert(mp->m_filestream, pino, &item->mru);
> - if (error)
> - goto out_free_item;
> + xfs_mru_cache_insert(mp->m_filestream, pino, &item->mru);
Hmm, don't you still need to check for -ENOMEM returns? Or if truly
none of the callers care anymore, then can we get rid of the return
value for xfs_mru_cache_insert?
--D
> return 0;
>
> -out_free_item:
> - xfs_perag_rele(item->pag);
> - kfree(item);
> out_put_fstrms:
> atomic_dec(&args->pag->pagf_fstrms);
> return 0;
> diff --git a/fs/xfs/xfs_mru_cache.c b/fs/xfs/xfs_mru_cache.c
> index d0f5b403bdbe..08443ceec329 100644
> --- a/fs/xfs/xfs_mru_cache.c
> +++ b/fs/xfs/xfs_mru_cache.c
> @@ -414,6 +414,8 @@ xfs_mru_cache_destroy(
> * To insert an element, call xfs_mru_cache_insert() with the data store, the
> * element's key and the client data pointer. This function returns 0 on
> * success or ENOMEM if memory for the data element couldn't be allocated.
> + *
> + * The passed in elem is freed through the per-cache free_func on failure.
> */
> int
> xfs_mru_cache_insert(
> @@ -421,14 +423,15 @@ xfs_mru_cache_insert(
> unsigned long key,
> struct xfs_mru_cache_elem *elem)
> {
> - int error;
> + int error = -EINVAL;
>
> ASSERT(mru && mru->lists);
> if (!mru || !mru->lists)
> - return -EINVAL;
> + goto out_free;
>
> + error = -ENOMEM;
> if (radix_tree_preload(GFP_KERNEL))
> - return -ENOMEM;
> + goto out_free;
>
> INIT_LIST_HEAD(&elem->list_node);
> elem->key = key;
> @@ -440,6 +443,12 @@ xfs_mru_cache_insert(
> _xfs_mru_cache_list_insert(mru, elem);
> spin_unlock(&mru->lock);
>
> + if (error)
> + goto out_free;
> + return 0;
> +
> +out_free:
> + mru->free_func(mru->data, elem);
> return error;
> }
>
> --
> 2.34.1
>
On Fri, May 02, 2025 at 01:06:46PM -0700, Darrick J. Wong wrote: > > atomic_inc(&pag_group(args->pag)->xg_active_ref); > > item->pag = args->pag; > > - error = xfs_mru_cache_insert(mp->m_filestream, pino, &item->mru); > > - if (error) > > - goto out_free_item; > > + xfs_mru_cache_insert(mp->m_filestream, pino, &item->mru); > > Hmm, don't you still need to check for -ENOMEM returns? Or if truly > none of the callers care anymore, then can we get rid of the return > value for xfs_mru_cache_insert? Both for file streams and the zone association in the next patch the mru cache is just a hint, so we ignore all errors (see the return 0 in the error handling boilerplate in the existing code). But hardcoding that assumption into the core mru cache helpers seems a bit weird.
On Mon, May 05, 2025 at 07:45:49AM +0200, hch wrote: > On Fri, May 02, 2025 at 01:06:46PM -0700, Darrick J. Wong wrote: > > > atomic_inc(&pag_group(args->pag)->xg_active_ref); > > > item->pag = args->pag; > > > - error = xfs_mru_cache_insert(mp->m_filestream, pino, &item->mru); > > > - if (error) > > > - goto out_free_item; > > > + xfs_mru_cache_insert(mp->m_filestream, pino, &item->mru); > > > > Hmm, don't you still need to check for -ENOMEM returns? Or if truly > > none of the callers care anymore, then can we get rid of the return > > value for xfs_mru_cache_insert? > > Both for file streams and the zone association in the next patch the > mru cache is just a hint, so we ignore all errors (see the return 0 > in the error handling boilerplate in the existing code). But hardcoding > that assumption into the core mru cache helpers seems a bit weird. Ok then. The comment change in this patch is a reasonable explanation for why the return value is/has always been ignored, so Reviewed-by: "Darrick J. Wong" <djwong@kernel.org> --D
© 2016 - 2026 Red Hat, Inc.