| Message ID | 20200510072404.986627-7-hch@lst.de (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [1/6] xfs: use XFS_IFORK_BOFF xchk_bmap_check_rmaps | expand |
On Sunday 10 May 2020 12:54:04 PM IST 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). > The changes look good to me. Reviewed-by: Chandan Babu R <chandanrlinux@gmail.com> > 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_attr_leaf.c b/fs/xfs/libxfs/xfs_attr_leaf.c > index d7f3173ce3c31..8d775942f1c6c 100644 > --- a/fs/xfs/libxfs/xfs_attr_leaf.c > +++ b/fs/xfs/libxfs/xfs_attr_leaf.c > @@ -716,11 +716,10 @@ xfs_attr_fork_remove( > struct xfs_inode *ip, > struct xfs_trans *tp) > { > - xfs_idestroy_fork(ip, XFS_ATTR_FORK); > + xfs_idestroy_fork(ip->i_afp); > + kmem_cache_free(xfs_ifork_zone, ip->i_afp); > + ip->i_afp = NULL; > ip->i_d.di_forkoff = 0; > - > - ASSERT(ip->i_afp == NULL); > - > xfs_trans_log_inode(tp, ip, XFS_ILOG_CORE); > } > > diff --git a/fs/xfs/libxfs/xfs_inode_buf.c b/fs/xfs/libxfs/xfs_inode_buf.c > index ab555671e1543..6f84ea85fdd83 100644 > --- a/fs/xfs/libxfs/xfs_inode_buf.c > +++ b/fs/xfs/libxfs/xfs_inode_buf.c > @@ -271,7 +271,7 @@ xfs_inode_from_disk( > return 0; > > out_destroy_data_fork: > - xfs_idestroy_fork(ip, XFS_DATA_FORK); > + xfs_idestroy_fork(&ip->i_df); > return error; > } > > 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); > > /* > - * 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)); >
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 >
On Tue, May 12, 2020 at 02:54:35PM -0400, Brian Foster wrote: > 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. /me agrees, let's not leave a potential UAF landmine here. --D > 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 > > >
diff --git a/fs/xfs/libxfs/xfs_attr_leaf.c b/fs/xfs/libxfs/xfs_attr_leaf.c index d7f3173ce3c31..8d775942f1c6c 100644 --- a/fs/xfs/libxfs/xfs_attr_leaf.c +++ b/fs/xfs/libxfs/xfs_attr_leaf.c @@ -716,11 +716,10 @@ xfs_attr_fork_remove( struct xfs_inode *ip, struct xfs_trans *tp) { - xfs_idestroy_fork(ip, XFS_ATTR_FORK); + xfs_idestroy_fork(ip->i_afp); + kmem_cache_free(xfs_ifork_zone, ip->i_afp); + ip->i_afp = NULL; ip->i_d.di_forkoff = 0; - - ASSERT(ip->i_afp == NULL); - xfs_trans_log_inode(tp, ip, XFS_ILOG_CORE); } diff --git a/fs/xfs/libxfs/xfs_inode_buf.c b/fs/xfs/libxfs/xfs_inode_buf.c index ab555671e1543..6f84ea85fdd83 100644 --- a/fs/xfs/libxfs/xfs_inode_buf.c +++ b/fs/xfs/libxfs/xfs_inode_buf.c @@ -271,7 +271,7 @@ xfs_inode_from_disk( return 0; out_destroy_data_fork: - xfs_idestroy_fork(ip, XFS_DATA_FORK); + xfs_idestroy_fork(&ip->i_df); return error; } 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); /* - * 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));
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(-)