From: Brian Foster <bfoster@redhat.com>
To: Christoph Hellwig <hch@lst.de>
Cc: linux-xfs@vger.kernel.org
Subject: Re: [PATCH 6/6] xfs: cleanup xfs_idestroy_fork
Date: Tue, 12 May 2020 14:54:35 -0400 [thread overview]
Message-ID: <20200512185435.GL37029@bfoster> (raw)
In-Reply-To: <20200510072404.986627-7-hch@lst.de>
On Sun, May 10, 2020 at 09:24:04AM +0200, Christoph Hellwig wrote:
> Move freeing the dynamically allocated attr and COW fork, as well
> as zeroing the pointers where actually needed into the callers, and
> just pass the xfs_ifork structure to xfs_idestroy_fork. Simplify
> the kmem_free calls by not checking for NULL first, and not zeroing
> the pointers in structure that are about to be freed (either the
> ifork or the containing inode in case of the data fork).
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
> fs/xfs/libxfs/xfs_attr_leaf.c | 7 +++----
> fs/xfs/libxfs/xfs_inode_buf.c | 2 +-
> fs/xfs/libxfs/xfs_inode_fork.c | 36 +++++++++-------------------------
> fs/xfs/libxfs/xfs_inode_fork.h | 2 +-
> fs/xfs/xfs_attr_inactive.c | 7 +++++--
> fs/xfs/xfs_icache.c | 15 ++++++++------
> 6 files changed, 28 insertions(+), 41 deletions(-)
>
...
> diff --git a/fs/xfs/libxfs/xfs_inode_fork.c b/fs/xfs/libxfs/xfs_inode_fork.c
> index 6562f2bcd15cc..577cc20e03170 100644
> --- a/fs/xfs/libxfs/xfs_inode_fork.c
> +++ b/fs/xfs/libxfs/xfs_inode_fork.c
> @@ -495,38 +495,20 @@ xfs_idata_realloc(
>
> void
> xfs_idestroy_fork(
> - xfs_inode_t *ip,
> - int whichfork)
> + struct xfs_ifork *ifp)
> {
> - struct xfs_ifork *ifp;
> -
> - ifp = XFS_IFORK_PTR(ip, whichfork);
> - if (ifp->if_broot != NULL) {
> - kmem_free(ifp->if_broot);
> - ifp->if_broot = NULL;
> - }
> + kmem_free(ifp->if_broot);
I think this function should still reset the pointers within the ifp
that it frees (if_broot and if_data below), particularly as long as
there are multiple callers that pass the data fork because it is not
immediately/independently freed. IOW, it's not clear if something
happens to reset i_mode between when xfs_inode_from_disk() might fail
and destroy the data fork, and when the inode is ultimately freed and we
look at i_mode to determine whether to destroy the data fork.
Brian
>
> /*
> - * If the format is local, then we can't have an extents
> - * array so just look for an inline data array. If we're
> - * not local then we may or may not have an extents list,
> - * so check and free it up if we do.
> + * If the format is local, then we can't have an extents array so just
> + * look for an inline data array. If we're not local then we may or may
> + * not have an extents list, so check and free it up if we do.
> */
> if (ifp->if_format == XFS_DINODE_FMT_LOCAL) {
> - if (ifp->if_u1.if_data != NULL) {
> - kmem_free(ifp->if_u1.if_data);
> - ifp->if_u1.if_data = NULL;
> - }
> - } else if ((ifp->if_flags & XFS_IFEXTENTS) && ifp->if_height) {
> - xfs_iext_destroy(ifp);
> - }
> -
> - if (whichfork == XFS_ATTR_FORK) {
> - kmem_cache_free(xfs_ifork_zone, ip->i_afp);
> - ip->i_afp = NULL;
> - } else if (whichfork == XFS_COW_FORK) {
> - kmem_cache_free(xfs_ifork_zone, ip->i_cowfp);
> - ip->i_cowfp = NULL;
> + kmem_free(ifp->if_u1.if_data);
> + } else if (ifp->if_flags & XFS_IFEXTENTS) {
> + if (ifp->if_height)
> + xfs_iext_destroy(ifp);
> }
> }
>
> diff --git a/fs/xfs/libxfs/xfs_inode_fork.h b/fs/xfs/libxfs/xfs_inode_fork.h
> index d849cca103edd..a4953e95c4f3f 100644
> --- a/fs/xfs/libxfs/xfs_inode_fork.h
> +++ b/fs/xfs/libxfs/xfs_inode_fork.h
> @@ -86,7 +86,7 @@ int xfs_iformat_data_fork(struct xfs_inode *, struct xfs_dinode *);
> int xfs_iformat_attr_fork(struct xfs_inode *, struct xfs_dinode *);
> void xfs_iflush_fork(struct xfs_inode *, struct xfs_dinode *,
> struct xfs_inode_log_item *, int);
> -void xfs_idestroy_fork(struct xfs_inode *, int);
> +void xfs_idestroy_fork(struct xfs_ifork *ifp);
> void xfs_idata_realloc(struct xfs_inode *ip, int64_t byte_diff,
> int whichfork);
> void xfs_iroot_realloc(struct xfs_inode *, int, int);
> diff --git a/fs/xfs/xfs_attr_inactive.c b/fs/xfs/xfs_attr_inactive.c
> index 00ffc46c0bf71..bfad669e6b2f8 100644
> --- a/fs/xfs/xfs_attr_inactive.c
> +++ b/fs/xfs/xfs_attr_inactive.c
> @@ -388,8 +388,11 @@ xfs_attr_inactive(
> xfs_trans_cancel(trans);
> out_destroy_fork:
> /* kill the in-core attr fork before we drop the inode lock */
> - if (dp->i_afp)
> - xfs_idestroy_fork(dp, XFS_ATTR_FORK);
> + if (dp->i_afp) {
> + xfs_idestroy_fork(dp->i_afp);
> + kmem_cache_free(xfs_ifork_zone, dp->i_afp);
> + dp->i_afp = NULL;
> + }
> if (lock_mode)
> xfs_iunlock(dp, lock_mode);
> return error;
> diff --git a/fs/xfs/xfs_icache.c b/fs/xfs/xfs_icache.c
> index c09b3e9eab1da..d806d3bfa8936 100644
> --- a/fs/xfs/xfs_icache.c
> +++ b/fs/xfs/xfs_icache.c
> @@ -87,15 +87,18 @@ xfs_inode_free_callback(
> case S_IFREG:
> case S_IFDIR:
> case S_IFLNK:
> - xfs_idestroy_fork(ip, XFS_DATA_FORK);
> + xfs_idestroy_fork(&ip->i_df);
> break;
> }
>
> - if (ip->i_afp)
> - xfs_idestroy_fork(ip, XFS_ATTR_FORK);
> - if (ip->i_cowfp)
> - xfs_idestroy_fork(ip, XFS_COW_FORK);
> -
> + if (ip->i_afp) {
> + xfs_idestroy_fork(ip->i_afp);
> + kmem_cache_free(xfs_ifork_zone, ip->i_afp);
> + }
> + if (ip->i_cowfp) {
> + xfs_idestroy_fork(ip->i_cowfp);
> + kmem_cache_free(xfs_ifork_zone, ip->i_cowfp);
> + }
> if (ip->i_itemp) {
> ASSERT(!test_bit(XFS_LI_IN_AIL,
> &ip->i_itemp->ili_item.li_flags));
> --
> 2.26.2
>
next prev parent reply other threads:[~2020-05-12 18:54 UTC|newest]
Thread overview: 41+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-05-10 7:23 move the extent count and format into struct xfs_ifork Christoph Hellwig
2020-05-10 7:23 ` [PATCH 1/6] xfs: use XFS_IFORK_BOFF xchk_bmap_check_rmaps Christoph Hellwig
2020-05-11 11:40 ` Chandan Babu R
2020-05-12 15:31 ` Brian Foster
2020-05-12 15:38 ` Darrick J. Wong
2020-05-12 16:14 ` Brian Foster
2020-05-12 19:16 ` Darrick J. Wong
2020-05-13 13:19 ` Brian Foster
2020-05-13 13:21 ` Christoph Hellwig
2020-05-16 18:42 ` [PATCH] xfs: clean up xchk_bmap_check_rmaps usage of XFS_IFORK_Q Darrick J. Wong
2020-05-16 20:01 ` Christoph Hellwig
2020-05-17 8:05 ` Christoph Hellwig
2020-05-10 7:24 ` [PATCH 2/6] xfs: remove the XFS_DFORK_Q macro Christoph Hellwig
2020-05-11 12:39 ` Chandan Babu R
2020-05-12 15:31 ` Brian Foster
2020-05-16 18:02 ` Darrick J. Wong
2020-05-10 7:24 ` [PATCH 3/6] xfs: remove xfs_ifree_local_data Christoph Hellwig
2020-05-11 16:32 ` Chandan Babu R
2020-05-12 15:31 ` Brian Foster
2020-05-16 18:07 ` Darrick J. Wong
2022-07-24 12:14 ` Alex Lyakas
2020-05-10 7:24 ` [PATCH 4/6] xfs: move the per-fork nextents fields into struct xfs_ifork Christoph Hellwig
2020-05-12 5:26 ` Chandan Babu R
2020-05-12 16:10 ` Brian Foster
2020-05-16 18:09 ` Darrick J. Wong
2020-05-17 8:12 ` Christoph Hellwig
2020-05-10 7:24 ` [PATCH 5/6] xfs: move the fork format " Christoph Hellwig
2020-05-12 9:37 ` Chandan Babu R
2020-05-12 18:53 ` Brian Foster
2020-05-14 21:25 ` Darrick J. Wong
2020-05-16 13:58 ` Christoph Hellwig
2020-05-16 17:01 ` Darrick J. Wong
2020-05-16 18:01 ` Darrick J. Wong
2020-05-16 18:16 ` Christoph Hellwig
2020-05-16 18:29 ` Darrick J. Wong
2020-05-10 7:24 ` [PATCH 6/6] xfs: cleanup xfs_idestroy_fork Christoph Hellwig
2020-05-12 9:48 ` Chandan Babu R
2020-05-12 18:54 ` Brian Foster [this message]
2020-05-16 18:10 ` Darrick J. Wong
2020-05-18 7:33 move the extent count and format into struct xfs_ifork v2 Christoph Hellwig
2020-05-18 7:33 ` [PATCH 6/6] xfs: cleanup xfs_idestroy_fork Christoph Hellwig
2020-05-18 13:28 ` Brian Foster
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20200512185435.GL37029@bfoster \
--to=bfoster@redhat.com \
--cc=hch@lst.de \
--cc=linux-xfs@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.