xfsprog: fix xfs_inode lifetime issue

PATCH In review Mark Tinguely 4 participants 10 messages Apr 30, 2014 – May 1, 2014

Share & tools

Mark Tinguely

author

2014-04-30 14:02 UTC #0 lore raw

cover xfsprog: fix xfs_inode lifetime issue

These patches clean up the xfs(_trans)_iput() and fix the xfs_inode life time so that inodes with a extended attribute fork can add an attributes.

Patch 1 removes the unused argument in _iput routines and
introduces the IRELSE define for xfs_iput().
Patch 2 fixes the xfs_inode lifetime problem.

Mark.

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

Mark Tinguely

author

2014-04-30 14:03 UTC #1 lore raw

PATCH 1/2 xfsprogs: remove unused argument in trans_iput

Remove the unused second argument to xfs_iput() and xfs_trans_iput().

Introduce the define "IRELE()" and use in place of xfs_iput().

Signed-off-by Mark Tinguely <[email protected]>

---
 db/attrset.c     |    4 ++--
 include/libxfs.h |    6 ++++--
 libxfs/init.c    |    4 ++--
 libxfs/rdwr.c    |    2 +-
 libxfs/trans.c   |   11 +++++------
 mkfs/proto.c     |    2 +-
 repair/phase6.c  |    2 +-
 repair/phase7.c  |    2 +-
 8 files changed, 17 insertions(+), 16 deletions(-)

db/attrset.c +2 -2

Index: b/db/attrset.c
===================================================================
@@ -170,7 +170,7 @@ attr_set_f(
 out:
 	mp->m_flags &= ~LIBXFS_MOUNT_COMPAT_ATTR;
 	if (ip)
-		libxfs_iput(ip, 0);
+		IRELE(ip);
 	if (value)
 		free(value);
 	return 0;

@@ -244,6 +244,6 @@ attr_remove_f(
 out:
 	mp->m_flags &= ~LIBXFS_MOUNT_COMPAT_ATTR;
 	if (ip)
-		libxfs_iput(ip, 0);
+		IRELE(ip);
 	return 0;
 }

include/libxfs.h +4 -2

Index: b/include/libxfs.h
===================================================================
@@ -532,7 +532,7 @@ extern xfs_buf_t	*libxfs_trans_getsb (xf
 
 extern int	libxfs_trans_iget (xfs_mount_t *, xfs_trans_t *, xfs_ino_t,
 				uint, uint, struct xfs_inode **);
-extern void	libxfs_trans_iput(xfs_trans_t *, struct xfs_inode *, uint);
+extern void	libxfs_trans_iput(xfs_trans_t *, struct xfs_inode *);
 extern void	libxfs_trans_ijoin (xfs_trans_t *, struct xfs_inode *, uint);
 extern void	libxfs_trans_ihold (xfs_trans_t *, struct xfs_inode *);
 extern void	libxfs_trans_ijoin_ref(xfs_trans_t *, struct xfs_inode *, int);

@@ -653,7 +653,9 @@ extern int	libxfs_iflush_int (xfs_inode_
 /* Inode Cache Interfaces */
 extern int	libxfs_iget (xfs_mount_t *, xfs_trans_t *, xfs_ino_t,
 				uint, xfs_inode_t **, xfs_daddr_t);
-extern void	libxfs_iput (xfs_inode_t *, uint);
+extern void	libxfs_iput (xfs_inode_t *);
+
+#define IRELE(ip) libxfs_iput(ip)
 
 /* Shared utility routines */
 extern unsigned int	libxfs_log2_roundup(unsigned int i);

libxfs/init.c +2 -2

Index: b/libxfs/init.c
===================================================================
@@ -778,9 +778,9 @@ void
 libxfs_rtmount_destroy(xfs_mount_t *mp)
 {
 	if (mp->m_rsumip)
-		libxfs_iput(mp->m_rsumip, 0);
+		IRELE(mp->m_rsumip);
 	if (mp->m_rbmip)
-		libxfs_iput(mp->m_rbmip, 0);
+		IRELE(mp->m_rbmip);
 	mp->m_rsumip = mp->m_rbmip = NULL;
 }

libxfs/rdwr.c +1 -1

Index: b/libxfs/rdwr.c
===================================================================
@@ -1076,7 +1076,7 @@ libxfs_idestroy(xfs_inode_t *ip)
 }
 
 void
-libxfs_iput(xfs_inode_t *ip, uint lock_flags)
+libxfs_iput(xfs_inode_t *ip)
 {
 	if (ip->i_itemp)
 		kmem_zone_free(xfs_ili_zone, ip->i_itemp);

libxfs/trans.c +5 -6

Index: b/libxfs/trans.c
===================================================================
@@ -250,13 +250,12 @@ libxfs_trans_iget(
 void
 libxfs_trans_iput(
 	xfs_trans_t		*tp,
-	xfs_inode_t		*ip,
-	uint			lock_flags)
+	xfs_inode_t		*ip)
 {
 	xfs_inode_log_item_t	*iip;
 
 	if (tp == NULL) {
-		libxfs_iput(ip, lock_flags);
+		IRELE(ip);
 		return;
 	}

@@ -265,7 +264,7 @@ libxfs_trans_iput(
 	ASSERT(iip != NULL);
 	xfs_trans_del_item(&iip->ili_item);
 
-	libxfs_iput(ip, lock_flags);
+	IRELE(ip);
 }
 
 void

@@ -737,7 +736,7 @@ ili_done:
 		return;
 	}
 	/* free the inode */
-	libxfs_iput(ip, 0);
+	IRELE(ip);
 }
 
 static void

@@ -819,7 +818,7 @@ inode_item_unlock(
 
 	iip->ili_flags = 0;
 	if (!iip->ili_lock_flags)
-		libxfs_iput(ip, 0);
+		IRELE(ip);
 	else
 		iip->ili_lock_flags = 0;
 }

mkfs/proto.c +1 -1

Index: b/mkfs/proto.c
===================================================================
@@ -589,7 +589,7 @@ parseproto(
 				break;
 			parseproto(mp, ip, fsxp, pp, name);
 		}
-		libxfs_iput(ip, 0);
+		IRELE(ip);
 		return;
 	default:
 		ASSERT(0);

repair/phase6.c +1 -1

Index: b/repair/phase6.c
===================================================================
@@ -2873,7 +2873,7 @@ process_dir_inode(
 					|XFS_TRANS_SYNC);
 		}
 	}
-	libxfs_iput(ip, 0);
+	IRELE(ip);
 }
 
 /*

repair/phase7.c +1 -1

Index: b/repair/phase7.c
===================================================================
@@ -99,7 +99,7 @@ update_inode_nlinks(
 	set_nlinks(&ip->i_d, ino, nlinks, &dirty);
 
 	if (!dirty)  {
-		libxfs_trans_iput(tp, ip, 0);
+		libxfs_trans_iput(tp, ip);
 		libxfs_trans_cancel(tp, XFS_TRANS_RELEASE_LOG_RES);
 	} else  {
 		libxfs_trans_log_inode(tp, ip, XFS_ILOG_CORE);

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

Eric Sandeen

2014-04-30 14:47 UTC #2 lore raw

Re: [PATCH 1/2] xfsprogs: remove unused argument in trans_iput

Quoted 4 lines · Mark Tinguely

Remove the unused second argument to xfs_iput() and xfs_trans_iput().
Introduce the define "IRELE()" and use in place of xfs_iput().

Why do this? We had been moving away from the upper-case-macro- redefined-to-a-function meme... what does this #define gain?

libxfs_iget/libxfs_iput pairs seem more obvious than libxfs_iget/IRELE()...

Thanks,
-Eric

Quoted 174 lines

Signed-off-by Mark Tinguely <[email protected]>

---
 db/attrset.c     |    4 ++--
 include/libxfs.h |    6 ++++--
 libxfs/init.c    |    4 ++--
 libxfs/rdwr.c    |    2 +-
 libxfs/trans.c   |   11 +++++------
 mkfs/proto.c     |    2 +-
 repair/phase6.c  |    2 +-
 repair/phase7.c  |    2 +-
 8 files changed, 17 insertions(+), 16 deletions(-)

Index: b/db/attrset.c
===================================================================

db/attrset.c +2 -2

@@ -170,7 +170,7 @@ attr_set_f(
 out:
 	mp->m_flags &= ~LIBXFS_MOUNT_COMPAT_ATTR;
 	if (ip)
-		libxfs_iput(ip, 0);
+		IRELE(ip);
 	if (value)
 		free(value);
 	return 0;

@@ -244,6 +244,6 @@ attr_remove_f(
 out:
 	mp->m_flags &= ~LIBXFS_MOUNT_COMPAT_ATTR;
 	if (ip)
-		libxfs_iput(ip, 0);
+		IRELE(ip);
 	return 0;
 }

Index: b/include/libxfs.h
===================================================================

include/libxfs.h +4 -2

@@ -532,7 +532,7 @@ extern xfs_buf_t	*libxfs_trans_getsb (xf
 
 extern int	libxfs_trans_iget (xfs_mount_t *, xfs_trans_t *, xfs_ino_t,
 				uint, uint, struct xfs_inode **);
-extern void	libxfs_trans_iput(xfs_trans_t *, struct xfs_inode *, uint);
+extern void	libxfs_trans_iput(xfs_trans_t *, struct xfs_inode *);
 extern void	libxfs_trans_ijoin (xfs_trans_t *, struct xfs_inode *, uint);
 extern void	libxfs_trans_ihold (xfs_trans_t *, struct xfs_inode *);
 extern void	libxfs_trans_ijoin_ref(xfs_trans_t *, struct xfs_inode *, int);

@@ -653,7 +653,9 @@ extern int	libxfs_iflush_int (xfs_inode_
 /* Inode Cache Interfaces */
 extern int	libxfs_iget (xfs_mount_t *, xfs_trans_t *, xfs_ino_t,
 				uint, xfs_inode_t **, xfs_daddr_t);
-extern void	libxfs_iput (xfs_inode_t *, uint);
+extern void	libxfs_iput (xfs_inode_t *);
+
+#define IRELE(ip) libxfs_iput(ip)
 
 /* Shared utility routines */
 extern unsigned int	libxfs_log2_roundup(unsigned int i);

Index: b/libxfs/init.c
===================================================================

libxfs/init.c +2 -2

@@ -778,9 +778,9 @@ void
 libxfs_rtmount_destroy(xfs_mount_t *mp)
 {
 	if (mp->m_rsumip)
-		libxfs_iput(mp->m_rsumip, 0);
+		IRELE(mp->m_rsumip);
 	if (mp->m_rbmip)
-		libxfs_iput(mp->m_rbmip, 0);
+		IRELE(mp->m_rbmip);
 	mp->m_rsumip = mp->m_rbmip = NULL;
 }

Index: b/libxfs/rdwr.c
===================================================================

libxfs/rdwr.c +1 -1

@@ -1076,7 +1076,7 @@ libxfs_idestroy(xfs_inode_t *ip)
 }
 
 void
-libxfs_iput(xfs_inode_t *ip, uint lock_flags)
+libxfs_iput(xfs_inode_t *ip)
 {
 	if (ip->i_itemp)
 		kmem_zone_free(xfs_ili_zone, ip->i_itemp);

Index: b/libxfs/trans.c
===================================================================

libxfs/trans.c +5 -6

@@ -250,13 +250,12 @@ libxfs_trans_iget(
 void
 libxfs_trans_iput(
 	xfs_trans_t		*tp,
-	xfs_inode_t		*ip,
-	uint			lock_flags)
+	xfs_inode_t		*ip)
 {
 	xfs_inode_log_item_t	*iip;
 
 	if (tp == NULL) {
-		libxfs_iput(ip, lock_flags);
+		IRELE(ip);
 		return;
 	}

@@ -265,7 +264,7 @@ libxfs_trans_iput(
 	ASSERT(iip != NULL);
 	xfs_trans_del_item(&iip->ili_item);
 
-	libxfs_iput(ip, lock_flags);
+	IRELE(ip);
 }
 
 void

@@ -737,7 +736,7 @@ ili_done:
 		return;
 	}
 	/* free the inode */
-	libxfs_iput(ip, 0);
+	IRELE(ip);
 }
 
 static void

@@ -819,7 +818,7 @@ inode_item_unlock(
 
 	iip->ili_flags = 0;
 	if (!iip->ili_lock_flags)
-		libxfs_iput(ip, 0);
+		IRELE(ip);
 	else
 		iip->ili_lock_flags = 0;
 }

Index: b/mkfs/proto.c
===================================================================

mkfs/proto.c +1 -1

@@ -589,7 +589,7 @@ parseproto(
 				break;
 			parseproto(mp, ip, fsxp, pp, name);
 		}
-		libxfs_iput(ip, 0);
+		IRELE(ip);
 		return;
 	default:
 		ASSERT(0);

Index: b/repair/phase6.c
===================================================================

repair/phase6.c +1 -1

@@ -2873,7 +2873,7 @@ process_dir_inode(
 					|XFS_TRANS_SYNC);
 		}
 	}
-	libxfs_iput(ip, 0);
+	IRELE(ip);
 }
 
 /*

Index: b/repair/phase7.c
===================================================================

repair/phase7.c +1 -1

@@ -99,7 +99,7 @@ update_inode_nlinks(
 	set_nlinks(&ip->i_d, ino, nlinks, &dirty);
 
 	if (!dirty)  {
-		libxfs_trans_iput(tp, ip, 0);
+		libxfs_trans_iput(tp, ip);
 		libxfs_trans_cancel(tp, XFS_TRANS_RELEASE_LOG_RES);
 	} else  {
 		libxfs_trans_log_inode(tp, ip, XFS_ILOG_CORE);

Quoted 5 lines

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

Mark Tinguely

author

2014-04-30 14:50 UTC #3 lore raw

Re: [PATCH 1/2] xfsprogs: remove unused argument in trans_iput

Quoted 5 lines · Eric Sandeen

Quoted 4 lines · Mark Tinguely
Quoted 4 lines
Remove the unused second argument to xfs_iput() and xfs_trans_iput().
Introduce the define "IRELE()" and use in place of xfs_iput().

Quoted 9 lines

Why do this? We had been moving away from the upper-case-macro- redefined-to-a-function meme... what does this #define gain?
libxfs_iget/libxfs_iput pairs seem more obvious than libxfs_iget/IRELE()...
Thanks,
-Eric

To be consistent with the kernel code.

--Mark.

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

Eric Sandeen

2014-04-30 15:02 UTC #4 lore raw

Re: [PATCH 1/2] xfsprogs: remove unused argument in trans_iput

Quoted 9 lines · Mark Tinguely

Quoted 8 lines · Eric Sandeen
Quoted 4 lines · Mark Tinguely
Quoted 4 lines
Remove the unused second argument to xfs_iput() and xfs_trans_iput().
Introduce the define "IRELE()" and use in place of xfs_iput().
Why do this? We had been moving away from the upper-case-macro- redefined-to-a-function meme... what does this #define gain?

Quoted 8 lines

Quoted 6 lines
libxfs_iget/libxfs_iput pairs seem more obvious than libxfs_iget/IRELE()...
Thanks,
-Eric
To be consistent with the kernel code.

Gah, hohum, I'm not even going to try to explain why I thought we didn't have it in kernel code.

sorry for the noise,
-Eric

Quoted 2 lines

--Mark.

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

Christoph Hellwig

2014-04-30 15:10 UTC #5 lore raw

Re: [PATCH 1/2] xfsprogs: remove unused argument in trans_iput

In reply to Eric Sandeen

Quoted 11 lines · Eric Sandeen

Quoted 4 lines · Mark Tinguely
Remove the unused second argument to xfs_iput() and xfs_trans_iput().
Introduce the define "IRELE()" and use in place of xfs_iput().
Why do this? We had been moving away from the upper-case-macro- redefined-to-a-function meme... what does this #define gain?
libxfs_iget/libxfs_iput pairs seem more obvious than libxfs_iget/IRELE()...

Mostly to match the kernel that makes it do an iput underneath..

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

Christoph Hellwig

Reviewed-by on v1 1/2

2014-04-30 15:11 UTC #6 lore raw

Re: [PATCH 1/2] xfsprogs: remove unused argument in trans_iput

In reply to Mark Tinguely

Quoted 6 lines · Mark Tinguely

Remove the unused second argument to xfs_iput() and xfs_trans_iput().
Introduce the define "IRELE()" and use in place of xfs_iput().
Signed-off-by Mark Tinguely <[email protected]>

Looks good,

Reviewed-by Christoph Hellwig <[email protected]>

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

Mark Tinguely

author

2014-04-30 14:04 UTC #7 lore raw

PATCH 2/2 xfsprogs: dont free xfs_inode until complete

In reply to Mark Tinguely

Originally, the xfs_inode are released upon the first call to xfs_trans_cancel, xfs_trans_commit, or inode_item_done. This code used the log item lock field to prevent the release of the inode on the next call to one of the above functions. This is a unusual use of the log item lock field which is suppose to specify which lock is to be release on transaction commit or cancel. User space does not perform locking in transactions..

Unfortunately, this breaks any code that relies on multiple transaction operations. For example, adding an extended attribute to an inode that does not have an attribute fork will fail:

 # xfs_db -x XFS_DEVICE
 xfs_db> inode INO_NUM
 xfs_db> attr_set newattribute

This patch does the following:
1) Removes the iput from the transaction completion and requires that the xfs_inode allocators call IRELE() when they are done with the pointer. The real time inodes are pointed to by the xfs_mount and have a longer lifetime.
2) Removes libxfs_trans_iput() because transaction entries are removed in transaction commit and cancel.
3) Removes libxfs_trans_ihold() which is an obsolete interface.
4) Removes the now unneeded ili_flags from the xfs_inode_log_item structure.

Signed-off-by Mark Tinguely <[email protected]>

---
 include/libxfs.h |    3 --
 libxfs/trans.c   |   59 +++++--------------------------------------------------
 libxfs/util.c    |    1 
 libxfs/xfs.h     |    1 
 mkfs/proto.c     |   16 +-------------
 repair/phase6.c  |   13 +++---------
 repair/phase7.c  |    2 -
 7 files changed, 13 insertions(+), 82 deletions(-)

include/libxfs.h +0 -3

Index: b/include/libxfs.h
===================================================================
@@ -477,7 +477,6 @@ typedef struct xfs_inode_log_item {
 	unsigned int		ili_fields;		/* fields to be logged */
 	unsigned int		ili_last_fields;	/* fields when flushed*/
 	xfs_inode_log_format_t	ili_format;		/* logged structure */
-	int			ili_lock_flags;
 } xfs_inode_log_item_t;
 
 typedef struct xfs_buf_log_item {

@@ -532,9 +531,7 @@ extern xfs_buf_t	*libxfs_trans_getsb (xf
 
 extern int	libxfs_trans_iget (xfs_mount_t *, xfs_trans_t *, xfs_ino_t,
 				uint, uint, struct xfs_inode **);
-extern void	libxfs_trans_iput(xfs_trans_t *, struct xfs_inode *);
 extern void	libxfs_trans_ijoin (xfs_trans_t *, struct xfs_inode *, uint);
-extern void	libxfs_trans_ihold (xfs_trans_t *, struct xfs_inode *);
 extern void	libxfs_trans_ijoin_ref(xfs_trans_t *, struct xfs_inode *, int);
 extern void	libxfs_trans_log_inode (xfs_trans_t *, struct xfs_inode *,
 				uint);

libxfs/trans.c +6 ~~-53~~

114 lines not on this page · raw message

libxfs/util.c +0 -1

Index: b/libxfs/util.c
===================================================================
@@ -595,7 +595,6 @@ libxfs_alloc_file_space(
 			break;
 		}
 		xfs_trans_ijoin(tp, ip, 0);
-		xfs_trans_ihold(tp, ip);
 
 		xfs_bmap_init(&free_list, &firstfsb);
 		error = xfs_bmapi_write(tp, ip, startoffset_fsb, allocatesize_fsb,

libxfs/xfs.h +0 -1

Index: b/libxfs/xfs.h
===================================================================
@@ -292,7 +292,6 @@ roundup_64(__uint64_t x, __uint32_t y)
 #define xfs_trans_get_buf		libxfs_trans_get_buf
 #define xfs_trans_getsb			libxfs_trans_getsb
 #define xfs_trans_iget			libxfs_trans_iget
-#define xfs_trans_ihold			libxfs_trans_ihold
 #define xfs_trans_ijoin			libxfs_trans_ijoin
 #define xfs_trans_ijoin_ref		libxfs_trans_ijoin_ref
 #define xfs_trans_init			libxfs_trans_init

mkfs/proto.c +2 ~~-14~~

Index: b/mkfs/proto.c
===================================================================
@@ -196,7 +196,6 @@ rsvfile(
 	tp = libxfs_trans_alloc(mp, 0);
 
 	libxfs_trans_ijoin(tp, ip, 0);
-	libxfs_trans_ihold(tp, ip);
 
 	ip->i_d.di_mode &= ~S_ISUID;

@@ -463,7 +462,6 @@ parseproto(
 		libxfs_trans_ijoin(tp, pip, 0);
 		xname.type = XFS_DIR3_FT_REG_FILE;
 		newdirent(mp, tp, pip, &xname, ip->i_ino, &first, &flist);
-		libxfs_trans_ihold(tp, pip);
 		break;
 
 	case IF_RESERVED:			/* pre-allocated space only */

@@ -480,7 +478,6 @@ parseproto(
 
 		xname.type = XFS_DIR3_FT_REG_FILE;
 		newdirent(mp, tp, pip, &xname, ip->i_ino, &first, &flist);
-		libxfs_trans_ihold(tp, pip);
 		libxfs_trans_log_inode(tp, ip, flags);
 
 		error = libxfs_bmap_finish(&tp, &flist, &committed);

@@ -488,6 +485,7 @@ parseproto(
 			fail(_("Pre-allocated file creation failed"), error);
 		libxfs_trans_commit(tp, 0);
 		rsvfile(mp, ip, llen);
+		IRELE(ip);
 		return;
 
 	case IF_BLOCK:

@@ -502,7 +500,6 @@ parseproto(
 		libxfs_trans_ijoin(tp, pip, 0);
 		xname.type = XFS_DIR3_FT_BLKDEV;
 		newdirent(mp, tp, pip, &xname, ip->i_ino, &first, &flist);
-		libxfs_trans_ihold(tp, pip);
 		flags |= XFS_ILOG_DEV;
 		break;

@@ -517,7 +514,6 @@ parseproto(
 		libxfs_trans_ijoin(tp, pip, 0);
 		xname.type = XFS_DIR3_FT_CHRDEV;
 		newdirent(mp, tp, pip, &xname, ip->i_ino, &first, &flist);
-		libxfs_trans_ihold(tp, pip);
 		flags |= XFS_ILOG_DEV;
 		break;

@@ -530,7 +526,6 @@ parseproto(
 		libxfs_trans_ijoin(tp, pip, 0);
 		xname.type = XFS_DIR3_FT_FIFO;
 		newdirent(mp, tp, pip, &xname, ip->i_ino, &first, &flist);
-		libxfs_trans_ihold(tp, pip);
 		break;
 	case IF_SYMLINK:
 		buf = getstr(pp);

@@ -544,7 +539,6 @@ parseproto(
 		libxfs_trans_ijoin(tp, pip, 0);
 		xname.type = XFS_DIR3_FT_SYMLINK;
 		newdirent(mp, tp, pip, &xname, ip->i_ino, &first, &flist);
-		libxfs_trans_ihold(tp, pip);
 		break;
 	case IF_DIRECTORY:
 		getres(tp, 0);

@@ -564,7 +558,6 @@ parseproto(
 			newdirent(mp, tp, pip, &xname, ip->i_ino,
 				  &first, &flist);
 			pip->i_d.di_nlink++;
-			libxfs_trans_ihold(tp, pip);
 			libxfs_trans_log_inode(tp, pip, XFS_ILOG_CORE);
 		}
 		newdirectory(mp, tp, ip, pip);

@@ -572,7 +565,6 @@ parseproto(
 		error = libxfs_bmap_finish(&tp, &flist, &committed);
 		if (error)
 			fail(_("Directory creation failed"), error);
-		libxfs_trans_ihold(tp, ip);
 		libxfs_trans_commit(tp, 0);
 		/*
 		 * RT initialization.  Do this here to ensure that

@@ -602,6 +594,7 @@ parseproto(
 			error);
 	}
 	libxfs_trans_commit(tp, 0);
+	IRELE(ip);
 }
 
 void

@@ -664,7 +657,6 @@ rtinit(
 	*(__uint64_t *)&rbmip->i_d.di_atime = 0;
 	libxfs_trans_log_inode(tp, rbmip, XFS_ILOG_CORE);
 	libxfs_mod_sb(tp, XFS_SB_RBMINO);
-	libxfs_trans_ihold(tp, rbmip);
 	mp->m_rbmip = rbmip;
 	error = libxfs_inode_alloc(&tp, NULL, S_IFREG, 1, 0,
 					&creds, &fsxattrs, &rsumip);

@@ -675,7 +667,6 @@ rtinit(
 	rsumip->i_d.di_size = mp->m_rsumsize;
 	libxfs_trans_log_inode(tp, rsumip, XFS_ILOG_CORE);
 	libxfs_mod_sb(tp, XFS_SB_RSUMINO);
-	libxfs_trans_ihold(tp, rsumip);
 	libxfs_trans_commit(tp, 0);
 	mp->m_rsumip = rsumip;
 	/*

@@ -688,7 +679,6 @@ rtinit(
 		res_failed(i);
 
 	libxfs_trans_ijoin(tp, rbmip, 0);
-	libxfs_trans_ihold(tp, rbmip);
 	bno = 0;
 	xfs_bmap_init(&flist, &first);
 	while (bno < mp->m_sb.sb_rbmblocks) {

@@ -725,7 +715,6 @@ rtinit(
 	if (i)
 		res_failed(i);
 	libxfs_trans_ijoin(tp, rsumip, 0);
-	libxfs_trans_ihold(tp, rsumip);
 	bno = 0;
 	xfs_bmap_init(&flist, &first);
 	while (bno < nsumblocks) {

@@ -761,7 +750,6 @@ rtinit(
 		if (i)
 			res_failed(i);
 		libxfs_trans_ijoin(tp, rbmip, 0);
-		libxfs_trans_ihold(tp, rbmip);
 		xfs_bmap_init(&flist, &first);
 		ebno = XFS_RTMIN(mp->m_sb.sb_rextents,
 			bno + NBBY * mp->m_sb.sb_blocksize);

repair/phase6.c +4 -9

Index: b/repair/phase6.c
===================================================================
@@ -505,7 +505,6 @@ mk_rbmino(xfs_mount_t *mp)
 	 * commit changes
 	 */
 	libxfs_trans_log_inode(tp, ip, XFS_ILOG_CORE);
-	libxfs_trans_ihold(tp, ip);
 	libxfs_trans_commit(tp, XFS_TRANS_RELEASE_LOG_RES|XFS_TRANS_SYNC);
 
 	/*

@@ -762,7 +761,6 @@ mk_rsumino(xfs_mount_t *mp)
 	 * commit changes
 	 */
 	libxfs_trans_log_inode(tp, ip, XFS_ILOG_CORE);
-	libxfs_trans_ihold(tp, ip);
 	libxfs_trans_commit(tp, XFS_TRANS_RELEASE_LOG_RES|XFS_TRANS_SYNC);
 
 	/*

@@ -1024,6 +1022,8 @@ mk_orphanage(xfs_mount_t *mp)
 
 
 	libxfs_trans_commit(tp, XFS_TRANS_RELEASE_LOG_RES|XFS_TRANS_SYNC);
+	IRELE(ip);
+	IRELE(pip);
 	add_inode_reached(irec,ino_offset);
 
 	return(ino);

@@ -1221,6 +1221,8 @@ mv_orphanage(
 
 		libxfs_trans_commit(tp, XFS_TRANS_RELEASE_LOG_RES|XFS_TRANS_SYNC);
 	}
+	IRELE(ino_p);
+	IRELE(orphanage_ip);
 }
 
 static int

@@ -1291,7 +1293,6 @@ longform_dir2_rebuild(
 	if (error)
 		res_failed(error);
 	libxfs_trans_ijoin(tp, ip, 0);
-	libxfs_trans_ihold(tp, ip);
 
 	if ((error = libxfs_bmap_last_offset(tp, ip, &lastblock,
 						XFS_DATA_FORK)))

@@ -1331,7 +1332,6 @@ longform_dir2_rebuild(
 			res_failed(error);
 
 		libxfs_trans_ijoin(tp, ip, 0);
-		libxfs_trans_ihold(tp, ip);
 
 		xfs_bmap_init(&flist, &firstblock);
 		error = libxfs_dir_createname(tp, ip, &p->name, p->inum,

@@ -1389,7 +1389,6 @@ dir2_kill_block(
 	if (error)
 		res_failed(error);
 	libxfs_trans_ijoin(tp, ip, 0);
-	libxfs_trans_ihold(tp, ip);
 	libxfs_trans_bjoin(tp, bp);
 	memset(&args, 0, sizeof(args));
 	xfs_bmap_init(&flist, &firstblock);

@@ -1577,7 +1576,6 @@ longform_dir2_entry_check_data(
 	if (error)
 		res_failed(error);
 	libxfs_trans_ijoin(tp, ip, 0);
-	libxfs_trans_ihold(tp, ip);
 	libxfs_trans_bjoin(tp, bp);
 	libxfs_trans_bhold(tp, bp);
 	xfs_bmap_init(&flist, &firstblock);

@@ -2744,7 +2742,6 @@ process_dir_inode(
 				res_failed(error);
 
 			libxfs_trans_ijoin(tp, ip, 0);
-			libxfs_trans_ihold(tp, ip);
 
 			shortform_dir2_entry_check(mp, ino, ip, &dirty,
 						irec, ino_offset,

@@ -2792,7 +2789,6 @@ process_dir_inode(
 			res_failed(error);
 
 		libxfs_trans_ijoin(tp, ip, 0);
-		libxfs_trans_ihold(tp, ip);
 
 		xfs_bmap_init(&flist, &first);

@@ -2854,7 +2850,6 @@ process_dir_inode(
 				res_failed(error);
 
 			libxfs_trans_ijoin(tp, ip, 0);
-			libxfs_trans_ihold(tp, ip);
 
 			xfs_bmap_init(&flist, &first);

repair/phase7.c +1 -1

Index: b/repair/phase7.c
===================================================================
@@ -99,7 +99,6 @@ update_inode_nlinks(
 	set_nlinks(&ip->i_d, ino, nlinks, &dirty);
 
 	if (!dirty)  {
-		libxfs_trans_iput(tp, ip);
 		libxfs_trans_cancel(tp, XFS_TRANS_RELEASE_LOG_RES);
 	} else  {
 		libxfs_trans_log_inode(tp, ip, XFS_ILOG_CORE);

@@ -113,6 +112,7 @@ update_inode_nlinks(
 
 		ASSERT(error == 0);
 	}
+	IRELE(ip);
 }
 
 void

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

Christoph Hellwig

Reviewed-by on v1 2/2

2014-04-30 15:14 UTC #8 lore raw

Re: [PATCH 2/2] xfsprogs: dont free xfs_inode until complete

Quoted 18 lines · Mark Tinguely

Originally, the xfs_inode are released upon the first call to xfs_trans_cancel, xfs_trans_commit, or inode_item_done. This code used the log item lock field to prevent the release of the inode on the next call to one of the above functions. This is a unusual use of the log item lock field which is suppose to specify which lock is to be release on transaction commit or cancel. User space does not perform locking in transactions..
Unfortunately, this breaks any code that relies on multiple transaction operations. For example, adding an extended attribute to an inode that does not have an attribute fork will fail:
 # xfs_db -x XFS_DEVICE
 xfs_db> inode INO_NUM
 xfs_db> attr_set newattribute

Quoted 6 lines

This patch does the following:
1) Removes the iput from the transaction completion and requires that the xfs_inode allocators call IRELE() when they are done with the pointer. The real time inodes are pointed to by the xfs_mount and have a longer lifetime.

Makes sense, the kernel hasn't done an iput during transaction completion for a long time.

Quoted 2 lines

2) Removes libxfs_trans_iput() because transaction entries are removed in transaction commit and cancel.

Also matches the kernel and makes sense.

Quoted 1 line

3) Removes libxfs_trans_ihold() which is an obsolete interface.

Ditto.

Quoted 2 lines

4) Removes the now unneeded ili_flags from the xfs_inode_log_item structure.

ili_lock_flags, not ili_flags. Given that we never lock inodes in userspace there is no need to unlock them during transaction commit, and no need to track this. There's also no shared code that references this field, so it can safely go.

The patch looks good to me, and having consistent behavior with the kernel code is very useful to have!

Reviewed-by Christoph Hellwig <[email protected]>

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

Dave Chinner

2014-05-01 23:34 UTC #9 lore raw

In reply to Mark Tinguely

Quoted 7 lines · Mark Tinguely

These patches clean up the xfs(_trans)_iput() and fix the xfs_inode life time so that inodes with a extended attribute fork can add an attributes.
Patch 1 removes the unused argument in _iput routines and
introduces the IRELSE define for xfs_iput().
Patch 2 fixes the xfs_inode lifetime problem.

These look OK, but I'm going to leave them until after the 3.2.0 release. Thanks for doing this, Mark!

Cheers,

Dave.

Signature · 7 lines

Dave Chinner
[email protected]

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs