[RFC PATCH 0/4] xfs: add O_TMPFILE support

[RFC PATCH 0/4] xfs: add O_TMPFILE support

[Zhi Yong Wu]

From: Zhi Yong Wu <wuzhy@linux.vnet.ibm.com>

HI, folks

The patchset is trying to add O_TMPFILE support to xfs. To make sure
that it is going ahead in the right direction, although it is only one
code draft, and hasn't strictly been tested, it is still post out ASAP.

For more information, please refer to Dave Chinner's RFD links:
  1.) http://oss.sgi.com/archives/xfs/2013-08/msg00339.html
  2.) http://oss.sgi.com/archives/xfs/2013-08/msg00336.html
  3.) http://oss.sgi.com/archives/xfs/2013-08/msg00341.html

Any comments are appreciated, thanks.

Zhi Yong Wu (4):
  xfs: adjust the interface of xfs_qm_vop_dqalloc()
  xfs: add xfs_create_tmpfile() for O_TMPFILE support
  xfs: add a new method xfs_vn_tmpfile()
  xfs: allow linkat() on O_TMPFILE files

 fs/xfs/xfs_inode.c       |  154 +++++++++++++++++++++++++++++++++++++++++++++-
 fs/xfs/xfs_inode.h       |    2 +
 fs/xfs/xfs_ioctl.c       |    2 +-
 fs/xfs/xfs_iops.c        |   24 +++++++-
 fs/xfs/xfs_qm.c          |   50 +++++++++------
 fs/xfs/xfs_quota.h       |    6 +-
 fs/xfs/xfs_shared.h      |    4 +-
 fs/xfs/xfs_symlink.c     |    2 +-
 fs/xfs/xfs_trans_resv.c  |   48 ++++++++++++++
 fs/xfs/xfs_trans_resv.h  |    4 +
 fs/xfs/xfs_trans_space.h |    2 +
 11 files changed, 270 insertions(+), 28 deletions(-)

-- 
1.7.6.5

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

[RFC PATCH 1/4] xfs: adjust the interface of xfs_qm_vop_dqalloc()

[Zhi Yong Wu]

From: Zhi Yong Wu <wuzhy@linux.vnet.ibm.com>

There isn't a parent inode or a name for O_TMPFILE support, but will pass
a struct xfs_mount to xfs_qm_vop_dqalloc(). So its interface need to be
adjusted in order that O_TMPFILE creation function can also use it.

Signed-off-by: Zhi Yong Wu <wuzhy@linux.vnet.ibm.com>
---
 fs/xfs/xfs_inode.c   |    2 +-
 fs/xfs/xfs_ioctl.c   |    2 +-
 fs/xfs/xfs_iops.c    |    3 ++-
 fs/xfs/xfs_qm.c      |   50 +++++++++++++++++++++++++++++++-------------------
 fs/xfs/xfs_quota.h   |    6 ++++--
 fs/xfs/xfs_symlink.c |    2 +-
 6 files changed, 40 insertions(+), 25 deletions(-)

diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
index 001aa89..1c23bdf 100644
--- a/fs/xfs/xfs_inode.c
+++ b/fs/xfs/xfs_inode.c
@@ -1177,7 +1177,7 @@ xfs_create(
 	/*
 	 * Make sure that we have allocated dquot(s) on disk.
 	 */
-	error = xfs_qm_vop_dqalloc(dp, xfs_kuid_to_uid(current_fsuid()),
+	error = xfs_qm_vop_dqalloc(dp, mp, xfs_kuid_to_uid(current_fsuid()),
 					xfs_kgid_to_gid(current_fsgid()), prid,
 					XFS_QMOPT_QUOTALL | XFS_QMOPT_INHERIT,
 					&udqp, &gdqp, &pdqp);
diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
index 4d61340..61abe32 100644
--- a/fs/xfs/xfs_ioctl.c
+++ b/fs/xfs/xfs_ioctl.c
@@ -1089,7 +1089,7 @@ xfs_ioctl_setattr(
 	 * because the i_*dquot fields will get updated anyway.
 	 */
 	if (XFS_IS_QUOTA_ON(mp) && (mask & FSX_PROJID)) {
-		code = xfs_qm_vop_dqalloc(ip, ip->i_d.di_uid,
+		code = xfs_qm_vop_dqalloc(ip, ip->i_mount, ip->i_d.di_uid,
 					 ip->i_d.di_gid, fa->fsx_projid,
 					 XFS_QMOPT_PQUOTA, &udqp, NULL, &pdqp);
 		if (code)
diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
index 27e0e54..eb55be5 100644
--- a/fs/xfs/xfs_iops.c
+++ b/fs/xfs/xfs_iops.c
@@ -540,7 +540,8 @@ xfs_setattr_nonsize(
 		 */
 		ASSERT(udqp == NULL);
 		ASSERT(gdqp == NULL);
-		error = xfs_qm_vop_dqalloc(ip, xfs_kuid_to_uid(uid),
+		error = xfs_qm_vop_dqalloc(ip, ip->i_mount,
+					   xfs_kuid_to_uid(uid),
 					   xfs_kgid_to_gid(gid),
 					   xfs_get_projid(ip),
 					   qflags, &udqp, &gdqp, NULL);
diff --git a/fs/xfs/xfs_qm.c b/fs/xfs/xfs_qm.c
index 14a4996..1f13e82 100644
--- a/fs/xfs/xfs_qm.c
+++ b/fs/xfs/xfs_qm.c
@@ -1765,6 +1765,7 @@ xfs_qm_write_sb_changes(
 int
 xfs_qm_vop_dqalloc(
 	struct xfs_inode	*ip,
+	struct xfs_mount	*mp,
 	xfs_dqid_t		uid,
 	xfs_dqid_t		gid,
 	prid_t			prid,
@@ -1773,7 +1774,6 @@ xfs_qm_vop_dqalloc(
 	struct xfs_dquot	**O_gdqpp,
 	struct xfs_dquot	**O_pdqpp)
 {
-	struct xfs_mount	*mp = ip->i_mount;
 	struct xfs_dquot	*uq = NULL;
 	struct xfs_dquot	*gq = NULL;
 	struct xfs_dquot	*pq = NULL;
@@ -1783,17 +1783,19 @@ xfs_qm_vop_dqalloc(
 	if (!XFS_IS_QUOTA_RUNNING(mp) || !XFS_IS_QUOTA_ON(mp))
 		return 0;
 
-	lockflags = XFS_ILOCK_EXCL;
-	xfs_ilock(ip, lockflags);
+	if (ip) {
+		lockflags = XFS_ILOCK_EXCL;
+		xfs_ilock(ip, lockflags);
 
-	if ((flags & XFS_QMOPT_INHERIT) && XFS_INHERIT_GID(ip))
-		gid = ip->i_d.di_gid;
+		if ((flags & XFS_QMOPT_INHERIT) && XFS_INHERIT_GID(ip))
+			gid = ip->i_d.di_gid;
+	}
 
 	/*
 	 * Attach the dquot(s) to this inode, doing a dquot allocation
 	 * if necessary. The dquot(s) will not be locked.
 	 */
-	if (XFS_NOT_DQATTACHED(mp, ip)) {
+	if (ip && XFS_NOT_DQATTACHED(mp, ip)) {
 		error = xfs_qm_dqattach_locked(ip, XFS_QMOPT_DQALLOC);
 		if (error) {
 			xfs_iunlock(ip, lockflags);
@@ -1802,7 +1804,7 @@ xfs_qm_vop_dqalloc(
 	}
 
 	if ((flags & XFS_QMOPT_UQUOTA) && XFS_IS_UQUOTA_ON(mp)) {
-		if (ip->i_d.di_uid != uid) {
+		if (ip || (ip->i_d.di_uid != uid)) {
 			/*
 			 * What we need is the dquot that has this uid, and
 			 * if we send the inode to dqget, the uid of the inode
@@ -1812,7 +1814,8 @@ xfs_qm_vop_dqalloc(
 			 * we'll deadlock by doing trans_reserve while
 			 * holding ilock.
 			 */
-			xfs_iunlock(ip, lockflags);
+			if (ip)
+				xfs_iunlock(ip, lockflags);
 			error = xfs_qm_dqget(mp, NULL, uid,
 						 XFS_DQ_USER,
 						 XFS_QMOPT_DQALLOC |
@@ -1826,8 +1829,10 @@ xfs_qm_vop_dqalloc(
 			 * Get the ilock in the right order.
 			 */
 			xfs_dqunlock(uq);
-			lockflags = XFS_ILOCK_SHARED;
-			xfs_ilock(ip, lockflags);
+			if (ip) {
+				lockflags = XFS_ILOCK_SHARED;
+				xfs_ilock(ip, lockflags);
+			}
 		} else {
 			/*
 			 * Take an extra reference, because we'll return
@@ -1838,8 +1843,9 @@ xfs_qm_vop_dqalloc(
 		}
 	}
 	if ((flags & XFS_QMOPT_GQUOTA) && XFS_IS_GQUOTA_ON(mp)) {
-		if (ip->i_d.di_gid != gid) {
-			xfs_iunlock(ip, lockflags);
+		if (ip && (ip->i_d.di_gid != gid)) {
+			if (ip)
+				xfs_iunlock(ip, lockflags);
 			error = xfs_qm_dqget(mp, NULL, gid,
 						 XFS_DQ_GROUP,
 						 XFS_QMOPT_DQALLOC |
@@ -1850,16 +1856,19 @@ xfs_qm_vop_dqalloc(
 				goto error_rele;
 			}
 			xfs_dqunlock(gq);
-			lockflags = XFS_ILOCK_SHARED;
-			xfs_ilock(ip, lockflags);
+			if (ip) {
+				lockflags = XFS_ILOCK_SHARED;
+				xfs_ilock(ip, lockflags);
+			}
 		} else {
 			ASSERT(ip->i_gdquot);
 			gq = xfs_qm_dqhold(ip->i_gdquot);
 		}
 	}
 	if ((flags & XFS_QMOPT_PQUOTA) && XFS_IS_PQUOTA_ON(mp)) {
-		if (xfs_get_projid(ip) != prid) {
-			xfs_iunlock(ip, lockflags);
+		if (ip || (xfs_get_projid(ip) != prid)) {
+			if (ip)
+				xfs_iunlock(ip, lockflags);
 			error = xfs_qm_dqget(mp, NULL, (xfs_dqid_t)prid,
 						 XFS_DQ_PROJ,
 						 XFS_QMOPT_DQALLOC |
@@ -1870,8 +1879,10 @@ xfs_qm_vop_dqalloc(
 				goto error_rele;
 			}
 			xfs_dqunlock(pq);
-			lockflags = XFS_ILOCK_SHARED;
-			xfs_ilock(ip, lockflags);
+			if (ip) {
+				lockflags = XFS_ILOCK_SHARED;
+				xfs_ilock(ip, lockflags);
+			}
 		} else {
 			ASSERT(ip->i_pdquot);
 			pq = xfs_qm_dqhold(ip->i_pdquot);
@@ -1880,7 +1891,8 @@ xfs_qm_vop_dqalloc(
 	if (uq)
 		trace_xfs_dquot_dqalloc(ip);
 
-	xfs_iunlock(ip, lockflags);
+	if (ip)
+		xfs_iunlock(ip, lockflags);
 	if (O_udqpp)
 		*O_udqpp = uq;
 	else if (uq)
diff --git a/fs/xfs/xfs_quota.h b/fs/xfs/xfs_quota.h
index 5376dd4..c898ad2 100644
--- a/fs/xfs/xfs_quota.h
+++ b/fs/xfs/xfs_quota.h
@@ -80,7 +80,8 @@ extern int xfs_trans_reserve_quota_bydquots(struct xfs_trans *,
 		struct xfs_mount *, struct xfs_dquot *,
 		struct xfs_dquot *, struct xfs_dquot *, long, long, uint);
 
-extern int xfs_qm_vop_dqalloc(struct xfs_inode *, xfs_dqid_t, xfs_dqid_t,
+extern int xfs_qm_vop_dqalloc(struct xfs_inode *, struct xfs_mount *,
+		xfs_dqid_t, xfs_dqid_t,
 		prid_t, uint, struct xfs_dquot **, struct xfs_dquot **,
 		struct xfs_dquot **);
 extern void xfs_qm_vop_create_dqattach(struct xfs_trans *, struct xfs_inode *,
@@ -103,7 +104,8 @@ extern void xfs_qm_unmount_quotas(struct xfs_mount *);
 
 #else
 static inline int
-xfs_qm_vop_dqalloc(struct xfs_inode *ip, xfs_dqid_t uid, xfs_dqid_t gid,
+xfs_qm_vop_dqalloc(struct xfs_inode *ip, struct xfs_mount *mp,
+		xfs_dqid_t uid, xfs_dqid_t gid,
 		prid_t prid, uint flags, struct xfs_dquot **udqp,
 		struct xfs_dquot **gdqp, struct xfs_dquot **pdqp)
 {
diff --git a/fs/xfs/xfs_symlink.c b/fs/xfs/xfs_symlink.c
index 14e58f2..dcb26692 100644
--- a/fs/xfs/xfs_symlink.c
+++ b/fs/xfs/xfs_symlink.c
@@ -216,7 +216,7 @@ xfs_symlink(
 	/*
 	 * Make sure that we have allocated dquot(s) on disk.
 	 */
-	error = xfs_qm_vop_dqalloc(dp,
+	error = xfs_qm_vop_dqalloc(dp, mp,
 			xfs_kuid_to_uid(current_fsuid()),
 			xfs_kgid_to_gid(current_fsgid()), prid,
 			XFS_QMOPT_QUOTALL | XFS_QMOPT_INHERIT,
-- 
1.7.6.5

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [RFC PATCH 1/4] xfs: adjust the interface of xfs_qm_vop_dqalloc()

[Christoph Hellwig]

On Mon, Nov 25, 2013 at 07:32:31PM +0800, Zhi Yong Wu wrote:
> From: Zhi Yong Wu <wuzhy@linux.vnet.ibm.com>
> 
> There isn't a parent inode or a name for O_TMPFILE support, but will pass
> a struct xfs_mount to xfs_qm_vop_dqalloc(). So its interface need to be
> adjusted in order that O_TMPFILE creation function can also use it.

Yes, there is.   The tmpfile method gets the parent directory passed
as it's first argument.

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [RFC PATCH 1/4] xfs: adjust the interface of xfs_qm_vop_dqalloc()

[Zhi Yong Wu]

On Mon, Nov 25, 2013 at 9:43 PM, Christoph Hellwig <hch@infradead.org> wrote:
> On Mon, Nov 25, 2013 at 07:32:31PM +0800, Zhi Yong Wu wrote:
>> From: Zhi Yong Wu <wuzhy@linux.vnet.ibm.com>
>>
>> There isn't a parent inode or a name for O_TMPFILE support, but will pass
>> a struct xfs_mount to xfs_qm_vop_dqalloc(). So its interface need to be
>> adjusted in order that O_TMPFILE creation function can also use it.
>
> Yes, there is.   The tmpfile method gets the parent directory passed
> as it's first argument.
ok, will pass it in.

>
> _______________________________________________
> xfs mailing list
> xfs@oss.sgi.com
> http://oss.sgi.com/mailman/listinfo/xfs



-- 
Regards,

Zhi Yong Wu

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

[RFC PATCH 2/4] xfs: add xfs_create_tmpfile() for O_TMPFILE support

[Zhi Yong Wu]

From: Zhi Yong Wu <wuzhy@linux.vnet.ibm.com>

The function is used to create one O_TMPFILE file.

http://oss.sgi.com/archives/xfs/2013-08/msg00339.html

Signed-off-by: Zhi Yong Wu <wuzhy@linux.vnet.ibm.com>
---
 fs/xfs/xfs_inode.c       |  129 ++++++++++++++++++++++++++++++++++++++++++++++
 fs/xfs/xfs_inode.h       |    2 +
 fs/xfs/xfs_shared.h      |    4 +-
 fs/xfs/xfs_trans_resv.c  |   35 ++++++++++++
 fs/xfs/xfs_trans_resv.h  |    2 +
 fs/xfs/xfs_trans_space.h |    2 +
 6 files changed, 173 insertions(+), 1 deletions(-)

diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
index 1c23bdf..e1832de 100644
--- a/fs/xfs/xfs_inode.c
+++ b/fs/xfs/xfs_inode.c
@@ -1337,6 +1337,135 @@ xfs_create(
 }
 
 int
+xfs_create_tmpfile(
+	xfs_mount_t		*mp,
+	umode_t			mode,
+	dev_t			rdev,
+	xfs_inode_t		**ipp)
+{
+	struct xfs_inode	*ip = NULL;
+	struct xfs_trans	*tp = NULL;
+	int			error;
+	uint			cancel_flags = XFS_TRANS_RELEASE_LOG_RES;
+	struct xfs_dquot	*udqp = NULL;
+	struct xfs_dquot	*gdqp = NULL;
+	struct xfs_dquot	*pdqp = NULL;
+	struct xfs_trans_res	*tres;
+	uint			resblks;
+
+	if (XFS_FORCED_SHUTDOWN(mp))
+		return XFS_ERROR(EIO);
+
+	/*
+	 * Make sure that we have allocated dquot(s) on disk.
+	 */
+	error = xfs_qm_vop_dqalloc(NULL, mp, xfs_kuid_to_uid(current_fsuid()),
+					xfs_kgid_to_gid(current_fsgid()),
+					XFS_PROJID_DEFAULT,
+					XFS_QMOPT_QUOTALL | XFS_QMOPT_INHERIT,
+					&udqp, &gdqp, &pdqp);
+	if (error)
+		return error;
+
+	resblks = XFS_CREATE_TMPFILE_SPACE_RES(mp);
+	tp = xfs_trans_alloc(mp, XFS_TRANS_CREATE_TMPFILE);
+
+	/*
+	 * Initially assume that the file does not exist and
+	 * reserve the resources for that case.  If that is not
+	 * the case we'll drop the one we have and get a more
+	 * appropriate transaction later.
+	 */
+	tres = &M_RES(mp)->tr_create_tmpfile;
+	error = xfs_trans_reserve(tp, tres, resblks, 0);
+	if (error == ENOSPC) {
+		/* flush outstanding delalloc blocks and retry */
+		xfs_flush_inodes(mp);
+		error = xfs_trans_reserve(tp, tres, resblks, 0);
+	}
+	if (error == ENOSPC) {
+		/* No space at all so try a "no-allocation" reservation */
+		resblks = 0;
+		error = xfs_trans_reserve(tp, tres, 0, 0);
+	}
+	if (error) {
+		cancel_flags = 0;
+		goto out_trans_cancel;
+	}
+
+	/*
+	 * Reserve disk quota and the inode.
+	 */
+	error = xfs_trans_reserve_quota(tp, mp, udqp, gdqp,
+						pdqp, resblks, 1, 0);
+	if (error)
+		goto out_trans_cancel;
+
+	/*
+	 * A newly created regular or special file just has one directory
+	 * entry pointing to them, but a directory also the "." entry
+	 * pointing to itself.
+	 */
+	error = xfs_dir_ialloc(&tp, NULL, mode, 1, rdev,
+				XFS_PROJID_DEFAULT, resblks > 0,
+				&ip, NULL);
+	if (error) {
+		if (error == ENOSPC)
+			goto out_trans_cancel;
+		goto out_trans_abort;
+	}
+
+	/*
+	 * If this is a synchronous mount, make sure that the
+	 * create transaction goes to disk before returning to
+	 * the user.
+	 */
+	if (mp->m_flags & (XFS_MOUNT_WSYNC|XFS_MOUNT_DIRSYNC))
+		xfs_trans_set_sync(tp);
+
+	/*
+	 * Attach the dquot(s) to the inodes and modify them incore.
+	 * These ids of the inode couldn't have changed since the new
+	 * inode has been locked ever since it was created.
+	 */
+	xfs_qm_vop_create_dqattach(tp, ip, udqp, gdqp, pdqp);
+
+	error = xfs_iunlink(tp, ip);
+	if (error)
+		goto out_trans_abort;
+
+	error = xfs_trans_commit(tp, XFS_TRANS_RELEASE_LOG_RES);
+	if (error)
+		goto out_release_inode;
+
+	xfs_qm_dqrele(udqp);
+	xfs_qm_dqrele(gdqp);
+	xfs_qm_dqrele(pdqp);
+
+	*ipp = ip;
+	return 0;
+
+ out_trans_abort:
+	cancel_flags |= XFS_TRANS_ABORT;
+ out_trans_cancel:
+	xfs_trans_cancel(tp, cancel_flags);
+ out_release_inode:
+	/*
+	 * Wait until after the current transaction is aborted to
+	 * release the inode.  This prevents recursive transactions
+	 * and deadlocks from xfs_inactive.
+	 */
+	if (ip)
+		IRELE(ip);
+
+	xfs_qm_dqrele(udqp);
+	xfs_qm_dqrele(gdqp);
+	xfs_qm_dqrele(pdqp);
+
+	return error;
+}
+
+int
 xfs_link(
 	xfs_inode_t		*tdp,
 	xfs_inode_t		*sip,
diff --git a/fs/xfs/xfs_inode.h b/fs/xfs/xfs_inode.h
index 9e6efccb..1f69fb3 100644
--- a/fs/xfs/xfs_inode.h
+++ b/fs/xfs/xfs_inode.h
@@ -323,6 +323,8 @@ int		xfs_lookup(struct xfs_inode *dp, struct xfs_name *name,
 			   struct xfs_inode **ipp, struct xfs_name *ci_name);
 int		xfs_create(struct xfs_inode *dp, struct xfs_name *name,
 			   umode_t mode, xfs_dev_t rdev, struct xfs_inode **ipp);
+int		xfs_create_tmpfile(struct xfs_mount *mp, umode_t mode,
+			   xfs_dev_t rdev, struct xfs_inode **ipp);
 int		xfs_remove(struct xfs_inode *dp, struct xfs_name *name,
 			   struct xfs_inode *ip);
 int		xfs_link(struct xfs_inode *tdp, struct xfs_inode *sip,
diff --git a/fs/xfs/xfs_shared.h b/fs/xfs/xfs_shared.h
index 8c5035a1..4484e51 100644
--- a/fs/xfs/xfs_shared.h
+++ b/fs/xfs/xfs_shared.h
@@ -104,7 +104,8 @@ extern const struct xfs_buf_ops xfs_symlink_buf_ops;
 #define	XFS_TRANS_SB_COUNT		41
 #define	XFS_TRANS_CHECKPOINT		42
 #define	XFS_TRANS_ICREATE		43
-#define	XFS_TRANS_TYPE_MAX		43
+#define	XFS_TRANS_CREATE_TMPFILE	44
+#define	XFS_TRANS_TYPE_MAX		44
 /* new transaction types need to be reflected in xfs_logprint(8) */
 
 #define XFS_TRANS_TYPES \
@@ -112,6 +113,7 @@ extern const struct xfs_buf_ops xfs_symlink_buf_ops;
 	{ XFS_TRANS_SETATTR_SIZE,	"SETATTR_SIZE" }, \
 	{ XFS_TRANS_INACTIVE,		"INACTIVE" }, \
 	{ XFS_TRANS_CREATE,		"CREATE" }, \
+	{ XFS_TRANS_CREATE_TMPFILE,	"CREATE_TMPFILE" }, \
 	{ XFS_TRANS_CREATE_TRUNC,	"CREATE_TRUNC" }, \
 	{ XFS_TRANS_TRUNCATE_FILE,	"TRUNCATE_FILE" }, \
 	{ XFS_TRANS_REMOVE,		"REMOVE" }, \
diff --git a/fs/xfs/xfs_trans_resv.c b/fs/xfs/xfs_trans_resv.c
index 2fd59c0..f1eebe4 100644
--- a/fs/xfs/xfs_trans_resv.c
+++ b/fs/xfs/xfs_trans_resv.c
@@ -343,6 +343,36 @@ xfs_calc_create_reservation(
 
 }
 
+STATIC uint
+xfs_calc_icreate_tmpfile_reservation(xfs_mount_t *mp)
+{
+	return XFS_DQUOT_LOGRES(mp) +
+		xfs_calc_icreate_resv_alloc(mp) +
+		xfs_calc_inode_res(mp, 1) +
+		xfs_calc_buf_res(1, XFS_FSB_TO_B(mp, 1)) +
+		xfs_calc_buf_res(2, mp->m_sb.sb_sectsize);
+}
+
+STATIC uint
+__xfs_calc_create_tmpfile_reservation(
+	struct xfs_mount	*mp)
+{
+	return XFS_DQUOT_LOGRES(mp) +
+		xfs_calc_create_resv_alloc(mp) +
+		xfs_calc_inode_res(mp, 1) +
+		xfs_calc_buf_res(1, XFS_FSB_TO_B(mp, 1)) +
+		xfs_calc_buf_res(2, mp->m_sb.sb_sectsize);
+}
+
+STATIC uint
+xfs_calc_create_tmpfile_reservation(
+	struct xfs_mount        *mp)
+{
+	if (xfs_sb_version_hascrc(&mp->m_sb))
+		return xfs_calc_icreate_tmpfile_reservation(mp);
+	return __xfs_calc_create_tmpfile_reservation(mp);
+}
+
 /*
  * Making a new directory is the same as creating a new file.
  */
@@ -729,6 +759,11 @@ xfs_trans_resv_calc(
 	resp->tr_create.tr_logcount = XFS_CREATE_LOG_COUNT;
 	resp->tr_create.tr_logflags |= XFS_TRANS_PERM_LOG_RES;
 
+	resp->tr_create_tmpfile.tr_logres =
+			xfs_calc_create_tmpfile_reservation(mp);
+	resp->tr_create_tmpfile.tr_logcount = XFS_CREATE_TMPFILE_LOG_COUNT;
+	resp->tr_create_tmpfile.tr_logflags |= XFS_TRANS_PERM_LOG_RES;
+
 	resp->tr_mkdir.tr_logres = xfs_calc_mkdir_reservation(mp);
 	resp->tr_mkdir.tr_logcount = XFS_MKDIR_LOG_COUNT;
 	resp->tr_mkdir.tr_logflags |= XFS_TRANS_PERM_LOG_RES;
diff --git a/fs/xfs/xfs_trans_resv.h b/fs/xfs/xfs_trans_resv.h
index de7de9a..285621d 100644
--- a/fs/xfs/xfs_trans_resv.h
+++ b/fs/xfs/xfs_trans_resv.h
@@ -38,6 +38,7 @@ struct xfs_trans_resv {
 	struct xfs_trans_res	tr_remove;	/* unlink trans */
 	struct xfs_trans_res	tr_symlink;	/* symlink trans */
 	struct xfs_trans_res	tr_create;	/* create trans */
+	struct xfs_trans_res	tr_create_tmpfile; /* create O_TMPFILE trans */
 	struct xfs_trans_res	tr_mkdir;	/* mkdir trans */
 	struct xfs_trans_res	tr_ifree;	/* inode free trans */
 	struct xfs_trans_res	tr_ichange;	/* inode update trans */
@@ -100,6 +101,7 @@ struct xfs_trans_resv {
 #define	XFS_ITRUNCATE_LOG_COUNT		2
 #define XFS_INACTIVE_LOG_COUNT		2
 #define	XFS_CREATE_LOG_COUNT		2
+#define	XFS_CREATE_TMPFILE_LOG_COUNT	2
 #define	XFS_MKDIR_LOG_COUNT		3
 #define	XFS_SYMLINK_LOG_COUNT		3
 #define	XFS_REMOVE_LOG_COUNT		2
diff --git a/fs/xfs/xfs_trans_space.h b/fs/xfs/xfs_trans_space.h
index 7d2c920..16fba89 100644
--- a/fs/xfs/xfs_trans_space.h
+++ b/fs/xfs/xfs_trans_space.h
@@ -61,6 +61,8 @@
 	(XFS_DAENTER_SPACE_RES(mp, XFS_ATTR_FORK) + XFS_B_TO_FSB(mp, v))
 #define	XFS_CREATE_SPACE_RES(mp,nl)	\
 	(XFS_IALLOC_SPACE_RES(mp) + XFS_DIRENTER_SPACE_RES(mp,nl))
+#define	XFS_CREATE_TMPFILE_SPACE_RES(mp)	\
+	XFS_IALLOC_SPACE_RES(mp)
 #define	XFS_DIOSTRAT_SPACE_RES(mp, v)	\
 	(XFS_EXTENTADD_SPACE_RES(mp, XFS_DATA_FORK) + (v))
 #define	XFS_GROWFS_SPACE_RES(mp)	\
-- 
1.7.6.5

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [RFC PATCH 2/4] xfs: add xfs_create_tmpfile() for O_TMPFILE support

[Christoph Hellwig]

On Mon, Nov 25, 2013 at 07:32:32PM +0800, Zhi Yong Wu wrote:
> From: Zhi Yong Wu <wuzhy@linux.vnet.ibm.com>
> 
> The function is used to create one O_TMPFILE file.
> 
> http://oss.sgi.com/archives/xfs/2013-08/msg00339.html
> 
> Signed-off-by: Zhi Yong Wu <wuzhy@linux.vnet.ibm.com>
> ---
>  fs/xfs/xfs_inode.c       |  129 ++++++++++++++++++++++++++++++++++++++++++++++
>  fs/xfs/xfs_inode.h       |    2 +
>  fs/xfs/xfs_shared.h      |    4 +-
>  fs/xfs/xfs_trans_resv.c  |   35 ++++++++++++
>  fs/xfs/xfs_trans_resv.h  |    2 +
>  fs/xfs/xfs_trans_space.h |    2 +
>  6 files changed, 173 insertions(+), 1 deletions(-)

>  int
> +xfs_create_tmpfile(
> +	xfs_mount_t		*mp,
> +	umode_t			mode,
> +	dev_t			rdev,
> +	xfs_inode_t		**ipp)

Please use struct xfs_mount and struct xfs_inode for any new code.

> +	/*
> +	 * Initially assume that the file does not exist and
> +	 * reserve the resources for that case.  If that is not
> +	 * the case we'll drop the one we have and get a more
> +	 * appropriate transaction later.
> +	 */

I can't see how this comment makes any sense.

> +	tres = &M_RES(mp)->tr_create_tmpfile;
> +	error = xfs_trans_reserve(tp, tres, resblks, 0);
> +	if (error == ENOSPC) {
> +		/* flush outstanding delalloc blocks and retry */
> +		xfs_flush_inodes(mp);
> +		error = xfs_trans_reserve(tp, tres, resblks, 0);
> +	}
> +	if (error == ENOSPC) {
> +		/* No space at all so try a "no-allocation" reservation */
> +		resblks = 0;
> +		error = xfs_trans_reserve(tp, tres, 0, 0);
> +	}

Please factor this into a new xfs_trans_reserver_create helper (better
names welcome of course).
similar.

> +	/*
> +	 * Reserve disk quota and the inode.
> +	 */

I don't think that comment adds a whole lot of value.  (Same for the
other quota comment above).

> +	/*
> +	 * A newly created regular or special file just has one directory
> +	 * entry pointing to them, but a directory also the "." entry
> +	 * pointing to itself.
> +	 */

Given that we only create regular files here the comment can be removed.

>  
> +STATIC uint
> +xfs_calc_icreate_tmpfile_reservation(xfs_mount_t *mp)
> +{
> +	return XFS_DQUOT_LOGRES(mp) +
> +		xfs_calc_icreate_resv_alloc(mp) +
> +		xfs_calc_inode_res(mp, 1) +
> +		xfs_calc_buf_res(1, XFS_FSB_TO_B(mp, 1)) +
> +		xfs_calc_buf_res(2, mp->m_sb.sb_sectsize);
> +}
> +
> +STATIC uint
> +__xfs_calc_create_tmpfile_reservation(
> +	struct xfs_mount	*mp)
> +{
> +	return XFS_DQUOT_LOGRES(mp) +
> +		xfs_calc_create_resv_alloc(mp) +
> +		xfs_calc_inode_res(mp, 1) +
> +		xfs_calc_buf_res(1, XFS_FSB_TO_B(mp, 1)) +
> +		xfs_calc_buf_res(2, mp->m_sb.sb_sectsize);
> +}
> +
> +STATIC uint
> +xfs_calc_create_tmpfile_reservation(
> +	struct xfs_mount        *mp)
> +{
> +	if (xfs_sb_version_hascrc(&mp->m_sb))
> +		return xfs_calc_icreate_tmpfile_reservation(mp);
> +	return __xfs_calc_create_tmpfile_reservation(mp);

Shouldn't we name this xfs_calc_create_tmpfile_reservation_v4 and _v5
or no postix and _crc?  Either way the double underscore naming looks
confusing.

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [RFC PATCH 2/4] xfs: add xfs_create_tmpfile() for O_TMPFILE support

[Zhi Yong Wu]

OK, will fix or cleanup all the comments, thanks.

On Mon, Nov 25, 2013 at 9:48 PM, Christoph Hellwig <hch@infradead.org> wrote:
> On Mon, Nov 25, 2013 at 07:32:32PM +0800, Zhi Yong Wu wrote:
>> From: Zhi Yong Wu <wuzhy@linux.vnet.ibm.com>
>>
>> The function is used to create one O_TMPFILE file.
>>
>> http://oss.sgi.com/archives/xfs/2013-08/msg00339.html
>>
>> Signed-off-by: Zhi Yong Wu <wuzhy@linux.vnet.ibm.com>
>> ---
>>  fs/xfs/xfs_inode.c       |  129 ++++++++++++++++++++++++++++++++++++++++++++++
>>  fs/xfs/xfs_inode.h       |    2 +
>>  fs/xfs/xfs_shared.h      |    4 +-
>>  fs/xfs/xfs_trans_resv.c  |   35 ++++++++++++
>>  fs/xfs/xfs_trans_resv.h  |    2 +
>>  fs/xfs/xfs_trans_space.h |    2 +
>>  6 files changed, 173 insertions(+), 1 deletions(-)
>
>>  int
>> +xfs_create_tmpfile(
>> +     xfs_mount_t             *mp,
>> +     umode_t                 mode,
>> +     dev_t                   rdev,
>> +     xfs_inode_t             **ipp)
>
> Please use struct xfs_mount and struct xfs_inode for any new code.
>
>> +     /*
>> +      * Initially assume that the file does not exist and
>> +      * reserve the resources for that case.  If that is not
>> +      * the case we'll drop the one we have and get a more
>> +      * appropriate transaction later.
>> +      */
>
> I can't see how this comment makes any sense.
>
>> +     tres = &M_RES(mp)->tr_create_tmpfile;
>> +     error = xfs_trans_reserve(tp, tres, resblks, 0);
>> +     if (error == ENOSPC) {
>> +             /* flush outstanding delalloc blocks and retry */
>> +             xfs_flush_inodes(mp);
>> +             error = xfs_trans_reserve(tp, tres, resblks, 0);
>> +     }
>> +     if (error == ENOSPC) {
>> +             /* No space at all so try a "no-allocation" reservation */
>> +             resblks = 0;
>> +             error = xfs_trans_reserve(tp, tres, 0, 0);
>> +     }
>
> Please factor this into a new xfs_trans_reserver_create helper (better
> names welcome of course).
> similar.
>
>> +     /*
>> +      * Reserve disk quota and the inode.
>> +      */
>
> I don't think that comment adds a whole lot of value.  (Same for the
> other quota comment above).
>
>> +     /*
>> +      * A newly created regular or special file just has one directory
>> +      * entry pointing to them, but a directory also the "." entry
>> +      * pointing to itself.
>> +      */
>
> Given that we only create regular files here the comment can be removed.
>
>>
>> +STATIC uint
>> +xfs_calc_icreate_tmpfile_reservation(xfs_mount_t *mp)
>> +{
>> +     return XFS_DQUOT_LOGRES(mp) +
>> +             xfs_calc_icreate_resv_alloc(mp) +
>> +             xfs_calc_inode_res(mp, 1) +
>> +             xfs_calc_buf_res(1, XFS_FSB_TO_B(mp, 1)) +
>> +             xfs_calc_buf_res(2, mp->m_sb.sb_sectsize);
>> +}
>> +
>> +STATIC uint
>> +__xfs_calc_create_tmpfile_reservation(
>> +     struct xfs_mount        *mp)
>> +{
>> +     return XFS_DQUOT_LOGRES(mp) +
>> +             xfs_calc_create_resv_alloc(mp) +
>> +             xfs_calc_inode_res(mp, 1) +
>> +             xfs_calc_buf_res(1, XFS_FSB_TO_B(mp, 1)) +
>> +             xfs_calc_buf_res(2, mp->m_sb.sb_sectsize);
>> +}
>> +
>> +STATIC uint
>> +xfs_calc_create_tmpfile_reservation(
>> +     struct xfs_mount        *mp)
>> +{
>> +     if (xfs_sb_version_hascrc(&mp->m_sb))
>> +             return xfs_calc_icreate_tmpfile_reservation(mp);
>> +     return __xfs_calc_create_tmpfile_reservation(mp);
>
> Shouldn't we name this xfs_calc_create_tmpfile_reservation_v4 and _v5
> or no postix and _crc?  Either way the double underscore naming looks
> confusing.
It follows up with the current naming style of regular file
reservation functions.
After we adopt Dave's suggestion, this issue will disappear.

>



-- 
Regards,

Zhi Yong Wu

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [RFC PATCH 2/4] xfs: add xfs_create_tmpfile() for O_TMPFILE support

[Dave Chinner]

On Mon, Nov 25, 2013 at 07:32:32PM +0800, Zhi Yong Wu wrote:
> From: Zhi Yong Wu <wuzhy@linux.vnet.ibm.com>
> 
> The function is used to create one O_TMPFILE file.
> 
> http://oss.sgi.com/archives/xfs/2013-08/msg00339.html
> 
> Signed-off-by: Zhi Yong Wu <wuzhy@linux.vnet.ibm.com>
> ---
>  fs/xfs/xfs_inode.c       |  129 ++++++++++++++++++++++++++++++++++++++++++++++
>  fs/xfs/xfs_inode.h       |    2 +
>  fs/xfs/xfs_shared.h      |    4 +-
>  fs/xfs/xfs_trans_resv.c  |   35 ++++++++++++
>  fs/xfs/xfs_trans_resv.h  |    2 +
>  fs/xfs/xfs_trans_space.h |    2 +
>  6 files changed, 173 insertions(+), 1 deletions(-)
> 
> diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
> index 1c23bdf..e1832de 100644
> --- a/fs/xfs/xfs_inode.c
> +++ b/fs/xfs/xfs_inode.c
> @@ -1337,6 +1337,135 @@ xfs_create(
>  }
>  
>  int
> +xfs_create_tmpfile(
> +	xfs_mount_t		*mp,
> +	umode_t			mode,
> +	dev_t			rdev,
> +	xfs_inode_t		**ipp)
> +{
> +	struct xfs_inode	*ip = NULL;
> +	struct xfs_trans	*tp = NULL;
> +	int			error;
> +	uint			cancel_flags = XFS_TRANS_RELEASE_LOG_RES;
> +	struct xfs_dquot	*udqp = NULL;
> +	struct xfs_dquot	*gdqp = NULL;
> +	struct xfs_dquot	*pdqp = NULL;
> +	struct xfs_trans_res	*tres;
> +	uint			resblks;
> +
> +	if (XFS_FORCED_SHUTDOWN(mp))
> +		return XFS_ERROR(EIO);
> +
> +	/*
> +	 * Make sure that we have allocated dquot(s) on disk.
> +	 */
> +	error = xfs_qm_vop_dqalloc(NULL, mp, xfs_kuid_to_uid(current_fsuid()),
> +					xfs_kgid_to_gid(current_fsgid()),
> +					XFS_PROJID_DEFAULT,
> +					XFS_QMOPT_QUOTALL | XFS_QMOPT_INHERIT,
> +					&udqp, &gdqp, &pdqp);
> +	if (error)
> +		return error;

Is XFS_PROJID_DEFAULT correct here? If we are getting a parent inode
from ->tmpfile, then this should be handled the same way as for
xfs_create.

> +
> +	resblks = XFS_CREATE_TMPFILE_SPACE_RES(mp);
> +	tp = xfs_trans_alloc(mp, XFS_TRANS_CREATE_TMPFILE);
> +
> +	/*
> +	 * Initially assume that the file does not exist and
> +	 * reserve the resources for that case.  If that is not
> +	 * the case we'll drop the one we have and get a more
> +	 * appropriate transaction later.
> +	 */
> +	tres = &M_RES(mp)->tr_create_tmpfile;
> +	error = xfs_trans_reserve(tp, tres, resblks, 0);
> +	if (error == ENOSPC) {
> +		/* flush outstanding delalloc blocks and retry */
> +		xfs_flush_inodes(mp);
> +		error = xfs_trans_reserve(tp, tres, resblks, 0);
> +	}
> +	if (error == ENOSPC) {
> +		/* No space at all so try a "no-allocation" reservation */
> +		resblks = 0;
> +		error = xfs_trans_reserve(tp, tres, 0, 0);
> +	}
> +	if (error) {
> +		cancel_flags = 0;
> +		goto out_trans_cancel;
> +	}

I don't think this is necessary here. The ENOSPC flushing in
xfs_create() is done to ensure we have space for directory block
creation, not so much for inode allocation. Hence it doesn't make a
lot of sense to have this here....

> +	/*
> +	 * Reserve disk quota and the inode.
> +	 */
> +	error = xfs_trans_reserve_quota(tp, mp, udqp, gdqp,
> +						pdqp, resblks, 1, 0);
> +	if (error)
> +		goto out_trans_cancel;
> +
> +	/*
> +	 * A newly created regular or special file just has one directory
> +	 * entry pointing to them, but a directory also the "." entry
> +	 * pointing to itself.
> +	 */
> +	error = xfs_dir_ialloc(&tp, NULL, mode, 1, rdev,
> +				XFS_PROJID_DEFAULT, resblks > 0,
> +				&ip, NULL);
> +	if (error) {
> +		if (error == ENOSPC)
> +			goto out_trans_cancel;
> +		goto out_trans_abort;
> +	}
> +
> +	/*
> +	 * If this is a synchronous mount, make sure that the
> +	 * create transaction goes to disk before returning to
> +	 * the user.
> +	 */
> +	if (mp->m_flags & (XFS_MOUNT_WSYNC|XFS_MOUNT_DIRSYNC))
> +		xfs_trans_set_sync(tp);

I'm not sure that XFS_MOUNT_DIRSYNC shoul dbe checked here, as there
is no directory operations to synchronise at all...

> +STATIC uint
> +xfs_calc_icreate_tmpfile_reservation(xfs_mount_t *mp)
> +{
> +	return XFS_DQUOT_LOGRES(mp) +
> +		xfs_calc_icreate_resv_alloc(mp) +
> +		xfs_calc_inode_res(mp, 1) +
> +		xfs_calc_buf_res(1, XFS_FSB_TO_B(mp, 1)) +
> +		xfs_calc_buf_res(2, mp->m_sb.sb_sectsize);
> +}
> +
> +STATIC uint
> +__xfs_calc_create_tmpfile_reservation(
> +	struct xfs_mount	*mp)
> +{
> +	return XFS_DQUOT_LOGRES(mp) +
> +		xfs_calc_create_resv_alloc(mp) +
> +		xfs_calc_inode_res(mp, 1) +
> +		xfs_calc_buf_res(1, XFS_FSB_TO_B(mp, 1)) +
> +		xfs_calc_buf_res(2, mp->m_sb.sb_sectsize);
> +}

What exactly is being modified here? All of the other reservations
have comments explaining what each of the individual components of
the reservation are. Yes, I can see that there's an inode, a
filesystem block and 2 sectors being reserved, but what are they?

It's also double counting the XFS_DQUOT_LOGRES(), as they are
accounted for in xfs_calc_[i]create_reservation().

Also, the directory create reservations are broken up into two
components - the inode allocation and the common directory name
creation component, and so the tmpfile reservation should probably
be broken up the same way. i.e this:

> +
> +STATIC uint
> +xfs_calc_create_tmpfile_reservation(
> +	struct xfs_mount        *mp)
> +{
> +	if (xfs_sb_version_hascrc(&mp->m_sb))
> +		return xfs_calc_icreate_tmpfile_reservation(mp);
> +	return __xfs_calc_create_tmpfile_reservation(mp);
> +}

Could simply be:

STATIC uint
xfs_calc_create_tmpfile_reservation(
	struct xfs_mount        *mp)
{
	uint	res;

	if (xfs_sb_version_hascrc(&mp->m_sb))
		res = xfs_calc_icreate_resv_alloc(mp);
	else
		res = xfs_calc_create_resv_alloc(mp);
	return res + xfs_calc_iunlink_add_resv(mp);
}


> diff --git a/fs/xfs/xfs_trans_space.h b/fs/xfs/xfs_trans_space.h
> index 7d2c920..16fba89 100644
> --- a/fs/xfs/xfs_trans_space.h
> +++ b/fs/xfs/xfs_trans_space.h
> @@ -61,6 +61,8 @@
>  	(XFS_DAENTER_SPACE_RES(mp, XFS_ATTR_FORK) + XFS_B_TO_FSB(mp, v))
>  #define	XFS_CREATE_SPACE_RES(mp,nl)	\
>  	(XFS_IALLOC_SPACE_RES(mp) + XFS_DIRENTER_SPACE_RES(mp,nl))
> +#define	XFS_CREATE_TMPFILE_SPACE_RES(mp)	\
> +	XFS_IALLOC_SPACE_RES(mp)

I wouldn't bother with this macro - just use XFS_IALLOC_SPACE_RES()
directly.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [RFC PATCH 2/4] xfs: add xfs_create_tmpfile() for O_TMPFILE support

[Christoph Hellwig]

On Tue, Nov 26, 2013 at 08:36:01AM +1100, Dave Chinner wrote:
> 
> Is XFS_PROJID_DEFAULT correct here? If we are getting a parent inode
> from ->tmpfile, then this should be handled the same way as for
> xfs_create.

It should.  And while we're at it that code from create and symlink
should be factored into:

static inline projid_t xfs_initial_projid(struct xfs_inode *dp)
{
	 if (dp->i_d.di_flags & XFS_DIFLAG_PROJINHERIT)
	 	return xfs_get_projid(dp);
	return XFS_PROJID_DEFAULT;
}

fist.

> I don't think this is necessary here. The ENOSPC flushing in
> xfs_create() is done to ensure we have space for directory block
> creation, not so much for inode allocation. Hence it doesn't make a
> lot of sense to have this here....

Point.  I take back my earlier comment that it should be factored.

> I'm not sure that XFS_MOUNT_DIRSYNC shoul dbe checked here, as there
> is no directory operations to synchronise at all...

It probably shouldn't indeed.  Reminds me of my old patch to make this
a flag to xfs_trans_commit instead of all that boilerplate code..

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [RFC PATCH 2/4] xfs: add xfs_create_tmpfile() for O_TMPFILE support

[Zhi Yong Wu]

On Tue, Nov 26, 2013 at 5:36 AM, Dave Chinner <david@fromorbit.com> wrote:
> On Mon, Nov 25, 2013 at 07:32:32PM +0800, Zhi Yong Wu wrote:
>> From: Zhi Yong Wu <wuzhy@linux.vnet.ibm.com>
>>
>> The function is used to create one O_TMPFILE file.
>>
>> http://oss.sgi.com/archives/xfs/2013-08/msg00339.html
>>
>> Signed-off-by: Zhi Yong Wu <wuzhy@linux.vnet.ibm.com>
>> ---
>>  fs/xfs/xfs_inode.c       |  129 ++++++++++++++++++++++++++++++++++++++++++++++
>>  fs/xfs/xfs_inode.h       |    2 +
>>  fs/xfs/xfs_shared.h      |    4 +-
>>  fs/xfs/xfs_trans_resv.c  |   35 ++++++++++++
>>  fs/xfs/xfs_trans_resv.h  |    2 +
>>  fs/xfs/xfs_trans_space.h |    2 +
>>  6 files changed, 173 insertions(+), 1 deletions(-)
>>
>> diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
>> index 1c23bdf..e1832de 100644
>> --- a/fs/xfs/xfs_inode.c
>> +++ b/fs/xfs/xfs_inode.c
>> @@ -1337,6 +1337,135 @@ xfs_create(
>>  }
>>
>>  int
>> +xfs_create_tmpfile(
>> +     xfs_mount_t             *mp,
>> +     umode_t                 mode,
>> +     dev_t                   rdev,
>> +     xfs_inode_t             **ipp)
>> +{
>> +     struct xfs_inode        *ip = NULL;
>> +     struct xfs_trans        *tp = NULL;
>> +     int                     error;
>> +     uint                    cancel_flags = XFS_TRANS_RELEASE_LOG_RES;
>> +     struct xfs_dquot        *udqp = NULL;
>> +     struct xfs_dquot        *gdqp = NULL;
>> +     struct xfs_dquot        *pdqp = NULL;
>> +     struct xfs_trans_res    *tres;
>> +     uint                    resblks;
>> +
>> +     if (XFS_FORCED_SHUTDOWN(mp))
>> +             return XFS_ERROR(EIO);
>> +
>> +     /*
>> +      * Make sure that we have allocated dquot(s) on disk.
>> +      */
>> +     error = xfs_qm_vop_dqalloc(NULL, mp, xfs_kuid_to_uid(current_fsuid()),
>> +                                     xfs_kgid_to_gid(current_fsgid()),
>> +                                     XFS_PROJID_DEFAULT,
>> +                                     XFS_QMOPT_QUOTALL | XFS_QMOPT_INHERIT,
>> +                                     &udqp, &gdqp, &pdqp);
>> +     if (error)
>> +             return error;
>
> Is XFS_PROJID_DEFAULT correct here? If we are getting a parent inode
> from ->tmpfile, then this should be handled the same way as for
> xfs_create.
ok, will fix it.
>
>> +
>> +     resblks = XFS_CREATE_TMPFILE_SPACE_RES(mp);
>> +     tp = xfs_trans_alloc(mp, XFS_TRANS_CREATE_TMPFILE);
>> +
>> +     /*
>> +      * Initially assume that the file does not exist and
>> +      * reserve the resources for that case.  If that is not
>> +      * the case we'll drop the one we have and get a more
>> +      * appropriate transaction later.
>> +      */
>> +     tres = &M_RES(mp)->tr_create_tmpfile;
>> +     error = xfs_trans_reserve(tp, tres, resblks, 0);
>> +     if (error == ENOSPC) {
>> +             /* flush outstanding delalloc blocks and retry */
>> +             xfs_flush_inodes(mp);
>> +             error = xfs_trans_reserve(tp, tres, resblks, 0);
>> +     }
>> +     if (error == ENOSPC) {
>> +             /* No space at all so try a "no-allocation" reservation */
>> +             resblks = 0;
>> +             error = xfs_trans_reserve(tp, tres, 0, 0);
>> +     }
>> +     if (error) {
>> +             cancel_flags = 0;
>> +             goto out_trans_cancel;
>> +     }
>
> I don't think this is necessary here. The ENOSPC flushing in
> xfs_create() is done to ensure we have space for directory block
> creation, not so much for inode allocation. Hence it doesn't make a
> lot of sense to have this here....
Good explaination, will fix it, thanks.
>
>> +     /*
>> +      * Reserve disk quota and the inode.
>> +      */
>> +     error = xfs_trans_reserve_quota(tp, mp, udqp, gdqp,
>> +                                             pdqp, resblks, 1, 0);
>> +     if (error)
>> +             goto out_trans_cancel;
>> +
>> +     /*
>> +      * A newly created regular or special file just has one directory
>> +      * entry pointing to them, but a directory also the "." entry
>> +      * pointing to itself.
>> +      */
>> +     error = xfs_dir_ialloc(&tp, NULL, mode, 1, rdev,
>> +                             XFS_PROJID_DEFAULT, resblks > 0,
>> +                             &ip, NULL);
>> +     if (error) {
>> +             if (error == ENOSPC)
>> +                     goto out_trans_cancel;
>> +             goto out_trans_abort;
>> +     }
>> +
>> +     /*
>> +      * If this is a synchronous mount, make sure that the
>> +      * create transaction goes to disk before returning to
>> +      * the user.
>> +      */
>> +     if (mp->m_flags & (XFS_MOUNT_WSYNC|XFS_MOUNT_DIRSYNC))
>> +             xfs_trans_set_sync(tp);
>
> I'm not sure that XFS_MOUNT_DIRSYNC shoul dbe checked here, as there
> is no directory operations to synchronise at all...
ok, will fix it.
>
>> +STATIC uint
>> +xfs_calc_icreate_tmpfile_reservation(xfs_mount_t *mp)
>> +{
>> +     return XFS_DQUOT_LOGRES(mp) +
>> +             xfs_calc_icreate_resv_alloc(mp) +
>> +             xfs_calc_inode_res(mp, 1) +
>> +             xfs_calc_buf_res(1, XFS_FSB_TO_B(mp, 1)) +
>> +             xfs_calc_buf_res(2, mp->m_sb.sb_sectsize);
>> +}
>> +
>> +STATIC uint
>> +__xfs_calc_create_tmpfile_reservation(
>> +     struct xfs_mount        *mp)
>> +{
>> +     return XFS_DQUOT_LOGRES(mp) +
>> +             xfs_calc_create_resv_alloc(mp) +
>> +             xfs_calc_inode_res(mp, 1) +
>> +             xfs_calc_buf_res(1, XFS_FSB_TO_B(mp, 1)) +
>> +             xfs_calc_buf_res(2, mp->m_sb.sb_sectsize);
>> +}
>
> What exactly is being modified here? All of the other reservations
> have comments explaining what each of the individual components of
> the reservation are. Yes, I can see that there's an inode, a
> filesystem block and 2 sectors being reserved, but what are they?
Will add some comments in next version.
>
> It's also double counting the XFS_DQUOT_LOGRES(), as they are
> accounted for in xfs_calc_[i]create_reservation().
I don't understand why you said it double counts XFS_DQUOT_LOGRES()? i
only see it is counted once. Can you say more details?

>
> Also, the directory create reservations are broken up into two
> components - the inode allocation and the common directory name
> creation component, and so the tmpfile reservation should probably
> be broken up the same way. i.e this:
>
>> +
>> +STATIC uint
>> +xfs_calc_create_tmpfile_reservation(
>> +     struct xfs_mount        *mp)
>> +{
>> +     if (xfs_sb_version_hascrc(&mp->m_sb))
>> +             return xfs_calc_icreate_tmpfile_reservation(mp);
>> +     return __xfs_calc_create_tmpfile_reservation(mp);
>> +}
>
> Could simply be:
>
> STATIC uint
> xfs_calc_create_tmpfile_reservation(
>         struct xfs_mount        *mp)
> {
>         uint    res;
>
>         if (xfs_sb_version_hascrc(&mp->m_sb))
>                 res = xfs_calc_icreate_resv_alloc(mp);
>         else
>                 res = xfs_calc_create_resv_alloc(mp);
>         return res + xfs_calc_iunlink_add_resv(mp);
> }
Good suggestion, will apply this.
>
>
>> diff --git a/fs/xfs/xfs_trans_space.h b/fs/xfs/xfs_trans_space.h
>> index 7d2c920..16fba89 100644
>> --- a/fs/xfs/xfs_trans_space.h
>> +++ b/fs/xfs/xfs_trans_space.h
>> @@ -61,6 +61,8 @@
>>       (XFS_DAENTER_SPACE_RES(mp, XFS_ATTR_FORK) + XFS_B_TO_FSB(mp, v))
>>  #define      XFS_CREATE_SPACE_RES(mp,nl)     \
>>       (XFS_IALLOC_SPACE_RES(mp) + XFS_DIRENTER_SPACE_RES(mp,nl))
>> +#define      XFS_CREATE_TMPFILE_SPACE_RES(mp)        \
>> +     XFS_IALLOC_SPACE_RES(mp)
>
> I wouldn't bother with this macro - just use XFS_IALLOC_SPACE_RES()
> directly.
ok, will remove this macro.

>
> Cheers,
>
> Dave.
> --
> Dave Chinner
> david@fromorbit.com



-- 
Regards,

Zhi Yong Wu

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

[RFC PATCH 3/4] xfs: add a new method xfs_vn_tmpfile()

[Zhi Yong Wu]

From: Zhi Yong Wu <wuzhy@linux.vnet.ibm.com>

Add a new O_TMPFILE method to VFS inteface.

http://oss.sgi.com/archives/xfs/2013-08/msg00336.html

Signed-off-by: Zhi Yong Wu <wuzhy@linux.vnet.ibm.com>
---
 fs/xfs/xfs_iops.c |   21 +++++++++++++++++++++
 1 files changed, 21 insertions(+), 0 deletions(-)

diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
index eb55be5..5d2e781 100644
--- a/fs/xfs/xfs_iops.c
+++ b/fs/xfs/xfs_iops.c
@@ -39,6 +39,7 @@
 #include "xfs_da_btree.h"
 #include "xfs_dir2_priv.h"
 #include "xfs_dinode.h"
+#include "xfs_trans_space.h"
 
 #include <linux/capability.h>
 #include <linux/xattr.h>
@@ -1051,6 +1052,24 @@ xfs_vn_fiemap(
 	return 0;
 }
 
+STATIC int
+xfs_vn_tmpfile(
+	struct inode	*dir,
+	struct dentry	*dentry,
+	umode_t		mode)
+{
+	struct xfs_inode *ip = NULL;
+	int		error;
+
+	error = xfs_create_tmpfile(XFS_I(dir)->i_mount, mode, 0, &ip);
+	if (error)
+		return -error;
+
+	d_instantiate(dentry, VFS_I(ip));
+
+	return -error;
+}
+
 static const struct inode_operations xfs_inode_operations = {
 	.get_acl		= xfs_get_acl,
 	.getattr		= xfs_vn_getattr,
@@ -1087,6 +1106,7 @@ static const struct inode_operations xfs_dir_inode_operations = {
 	.removexattr		= generic_removexattr,
 	.listxattr		= xfs_vn_listxattr,
 	.update_time		= xfs_vn_update_time,
+	.tmpfile		= xfs_vn_tmpfile,
 };
 
 static const struct inode_operations xfs_dir_ci_inode_operations = {
@@ -1113,6 +1133,7 @@ static const struct inode_operations xfs_dir_ci_inode_operations = {
 	.removexattr		= generic_removexattr,
 	.listxattr		= xfs_vn_listxattr,
 	.update_time		= xfs_vn_update_time,
+	.tmpfile		= xfs_vn_tmpfile,
 };
 
 static const struct inode_operations xfs_symlink_inode_operations = {
-- 
1.7.6.5

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [RFC PATCH 3/4] xfs: add a new method xfs_vn_tmpfile()

[Christoph Hellwig]

On Mon, Nov 25, 2013 at 07:32:33PM +0800, Zhi Yong Wu wrote:
> From: Zhi Yong Wu <wuzhy@linux.vnet.ibm.com>
> 
> Add a new O_TMPFILE method to VFS inteface.

Looks reasonable except that you should pass on the parent inode.

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [RFC PATCH 3/4] xfs: add a new method xfs_vn_tmpfile()

[Zhi Yong Wu]

On Mon, Nov 25, 2013 at 9:48 PM, Christoph Hellwig <hch@infradead.org> wrote:
> On Mon, Nov 25, 2013 at 07:32:33PM +0800, Zhi Yong Wu wrote:
>> From: Zhi Yong Wu <wuzhy@linux.vnet.ibm.com>
>>
>> Add a new O_TMPFILE method to VFS inteface.
>
> Looks reasonable except that you should pass on the parent inode.
ok, will do as this.
>



-- 
Regards,

Zhi Yong Wu

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

[RFC PATCH 4/4] xfs: allow linkat() on O_TMPFILE files

[Zhi Yong Wu]

From: Zhi Yong Wu <wuzhy@linux.vnet.ibm.com>

Enable O_TMPFILE support in linkat().

http://oss.sgi.com/archives/xfs/2013-08/msg00341.html

Signed-off-by: Zhi Yong Wu <wuzhy@linux.vnet.ibm.com>
---
 fs/xfs/xfs_inode.c      |   23 +++++++++++++++++++++--
 fs/xfs/xfs_trans_resv.c |   13 +++++++++++++
 fs/xfs/xfs_trans_resv.h |    2 ++
 3 files changed, 36 insertions(+), 2 deletions(-)

diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
index e1832de..7ab029b 100644
--- a/fs/xfs/xfs_inode.c
+++ b/fs/xfs/xfs_inode.c
@@ -62,6 +62,8 @@ kmem_zone_t *xfs_inode_zone;
 
 STATIC int xfs_iflush_int(xfs_inode_t *, xfs_buf_t *);
 
+STATIC int xfs_iunlink_remove(xfs_trans_t *, xfs_inode_t *);
+
 /*
  * helper function to extract extent size hint from inode
  */
@@ -1119,7 +1121,9 @@ xfs_bumplink(
 {
 	xfs_trans_ichgtime(tp, ip, XFS_ICHGTIME_CHG);
 
-	ASSERT(ip->i_d.di_nlink > 0);
+	if ((VFS_I(ip)->i_nlink == 0) &&
+	    !(VFS_I(ip)->i_state & I_LINKABLE))
+		ASSERT(ip->i_d.di_nlink > 0);
 	ip->i_d.di_nlink++;
 	inc_nlink(VFS_I(ip));
 	if ((ip->i_d.di_version == 1) &&
@@ -1473,6 +1477,7 @@ xfs_link(
 {
 	xfs_mount_t		*mp = tdp->i_mount;
 	xfs_trans_t		*tp;
+	struct xfs_trans_res	*tres;
 	int			error;
 	xfs_bmap_free_t         free_list;
 	xfs_fsblock_t           first_block;
@@ -1498,7 +1503,14 @@ xfs_link(
 	tp = xfs_trans_alloc(mp, XFS_TRANS_LINK);
 	cancel_flags = XFS_TRANS_RELEASE_LOG_RES;
 	resblks = XFS_LINK_SPACE_RES(mp, target_name->len);
-	error = xfs_trans_reserve(tp, &M_RES(mp)->tr_link, resblks, 0);
+
+	if ((VFS_I(sip)->i_nlink == 0) &&
+	    (VFS_I(sip)->i_state & I_LINKABLE))
+		tres = &M_RES(mp)->tr_link_tmpfile;
+	else
+		tres = &M_RES(mp)->tr_link;
+
+	error = xfs_trans_reserve(tp, tres, resblks, 0);
 	if (error == ENOSPC) {
 		resblks = 0;
 		error = xfs_trans_reserve(tp, &M_RES(mp)->tr_link, 0, 0);
@@ -1530,6 +1542,13 @@ xfs_link(
 
 	xfs_bmap_init(&free_list, &first_block);
 
+	if ((VFS_I(sip)->i_nlink == 0) &&
+	    (VFS_I(sip)->i_state & I_LINKABLE)) {
+		error = xfs_iunlink_remove(tp, sip);
+		if (error)
+			goto abort_return;
+	}
+
 	error = xfs_dir_createname(tp, tdp, target_name, sip->i_ino,
 					&first_block, &free_list, resblks);
 	if (error)
diff --git a/fs/xfs/xfs_trans_resv.c b/fs/xfs/xfs_trans_resv.c
index f1eebe4..7c1234e 100644
--- a/fs/xfs/xfs_trans_resv.c
+++ b/fs/xfs/xfs_trans_resv.c
@@ -228,6 +228,15 @@ xfs_calc_link_reservation(
 				      XFS_FSB_TO_B(mp, 1))));
 }
 
+STATIC uint
+xfs_calc_link_tmpfile_reservation(
+	struct xfs_mount        *mp)
+{
+	return xfs_calc_link_reservation(mp) +
+		xfs_calc_buf_res(2, XFS_FSB_TO_B(mp, 1)) +
+		xfs_calc_buf_res(2, mp->m_sb.sb_sectsize);
+}
+
 /*
  * For removing a directory entry we can modify:
  *    the parent directory inode: inode size
@@ -747,6 +756,10 @@ xfs_trans_resv_calc(
 	resp->tr_link.tr_logcount = XFS_LINK_LOG_COUNT;
 	resp->tr_link.tr_logflags |= XFS_TRANS_PERM_LOG_RES;
 
+	resp->tr_link_tmpfile.tr_logres = xfs_calc_link_tmpfile_reservation(mp);
+	resp->tr_link_tmpfile.tr_logcount = XFS_LINK_TMPFILE_LOG_COUNT;
+	resp->tr_link_tmpfile.tr_logflags |= XFS_TRANS_PERM_LOG_RES;
+
 	resp->tr_remove.tr_logres = xfs_calc_remove_reservation(mp);
 	resp->tr_remove.tr_logcount = XFS_REMOVE_LOG_COUNT;
 	resp->tr_remove.tr_logflags |= XFS_TRANS_PERM_LOG_RES;
diff --git a/fs/xfs/xfs_trans_resv.h b/fs/xfs/xfs_trans_resv.h
index 285621d..86a0daf 100644
--- a/fs/xfs/xfs_trans_resv.h
+++ b/fs/xfs/xfs_trans_resv.h
@@ -35,6 +35,7 @@ struct xfs_trans_resv {
 	struct xfs_trans_res	tr_itruncate;	/* truncate trans */
 	struct xfs_trans_res	tr_rename;	/* rename trans */
 	struct xfs_trans_res	tr_link;	/* link trans */
+	struct xfs_trans_res	tr_link_tmpfile; /* link O_TMPFILE trans */
 	struct xfs_trans_res	tr_remove;	/* unlink trans */
 	struct xfs_trans_res	tr_symlink;	/* symlink trans */
 	struct xfs_trans_res	tr_create;	/* create trans */
@@ -106,6 +107,7 @@ struct xfs_trans_resv {
 #define	XFS_SYMLINK_LOG_COUNT		3
 #define	XFS_REMOVE_LOG_COUNT		2
 #define	XFS_LINK_LOG_COUNT		2
+#define	XFS_LINK_TMPFILE_LOG_COUNT	2
 #define	XFS_RENAME_LOG_COUNT		2
 #define	XFS_WRITE_LOG_COUNT		2
 #define	XFS_ADDAFORK_LOG_COUNT		2
-- 
1.7.6.5

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [RFC PATCH 4/4] xfs: allow linkat() on O_TMPFILE files

[Christoph Hellwig]

> -	ASSERT(ip->i_d.di_nlink > 0);
> +	if ((VFS_I(ip)->i_nlink == 0) &&
> +	    !(VFS_I(ip)->i_state & I_LINKABLE))
> +		ASSERT(ip->i_d.di_nlink > 0);

	ASSERT(ip->i_d.di_nlink > 0 || (VFS_I(ip)->i_state & I_LINKABLE));

> @@ -1498,7 +1503,14 @@ xfs_link(
>  	tp = xfs_trans_alloc(mp, XFS_TRANS_LINK);
>  	cancel_flags = XFS_TRANS_RELEASE_LOG_RES;
>  	resblks = XFS_LINK_SPACE_RES(mp, target_name->len);
> -	error = xfs_trans_reserve(tp, &M_RES(mp)->tr_link, resblks, 0);
> +
> +	if ((VFS_I(sip)->i_nlink == 0) &&
> +	    (VFS_I(sip)->i_state & I_LINKABLE))
> +		tres = &M_RES(mp)->tr_link_tmpfile;
> +	else
> +		tres = &M_RES(mp)->tr_link;

Just check i_nlink, and for consistency it might make sense to just use
the xfs_inode one.  The VFS already made sure we don't inodes with
I_LINKABLE and a zero link count.

> +	if ((VFS_I(sip)->i_nlink == 0) &&
> +	    (VFS_I(sip)->i_state & I_LINKABLE)) {

Same here.

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [RFC PATCH 4/4] xfs: allow linkat() on O_TMPFILE files

[Zhi Yong Wu]

On Mon, Nov 25, 2013 at 9:51 PM, Christoph Hellwig <hch@infradead.org> wrote:
>> -     ASSERT(ip->i_d.di_nlink > 0);
>> +     if ((VFS_I(ip)->i_nlink == 0) &&
>> +         !(VFS_I(ip)->i_state & I_LINKABLE))
>> +             ASSERT(ip->i_d.di_nlink > 0);
>
>         ASSERT(ip->i_d.di_nlink > 0 || (VFS_I(ip)->i_state & I_LINKABLE));
This is wrong, and it should be
          ASSERT(ip->i_d.di_nlink > 0 || !(VFS_I(ip)->i_state & I_LINKABLE));

>
>> @@ -1498,7 +1503,14 @@ xfs_link(
>>       tp = xfs_trans_alloc(mp, XFS_TRANS_LINK);
>>       cancel_flags = XFS_TRANS_RELEASE_LOG_RES;
>>       resblks = XFS_LINK_SPACE_RES(mp, target_name->len);
>> -     error = xfs_trans_reserve(tp, &M_RES(mp)->tr_link, resblks, 0);
>> +
>> +     if ((VFS_I(sip)->i_nlink == 0) &&
>> +         (VFS_I(sip)->i_state & I_LINKABLE))
>> +             tres = &M_RES(mp)->tr_link_tmpfile;
>> +     else
>> +             tres = &M_RES(mp)->tr_link;
>
> Just check i_nlink, and for consistency it might make sense to just use
> the xfs_inode one.  The VFS already made sure we don't inodes with
but struct xfs_inode has no stuff similar to i_nlink....

> I_LINKABLE and a zero link count.
No, pls see the chunk of code:
int vfs_link(struct dentry *old_dentry, struct inode *dir, struct
dentry *new_dentry)
{
...
    /* Make sure we don't allow creating hardlink to an unlinked file */
    if (inode->i_nlink == 0 && !(inode->i_state & I_LINKABLE))
        error =  -ENOENT;
    else if (max_links && inode->i_nlink >= max_links)
        error = -EMLINK;
    else
        error = dir->i_op->link(old_dentry, dir, new_dentry);

    if (!error && (inode->i_state & I_LINKABLE)) {
        spin_lock(&inode->i_lock);
        inode->i_state &= ~I_LINKABLE;
        spin_unlock(&inode->i_lock);
    }
...
}

When xfs_link() is called, I_LINKABLE flag isn't cleared.

>
>> +     if ((VFS_I(sip)->i_nlink == 0) &&
>> +         (VFS_I(sip)->i_state & I_LINKABLE)) {
>
> Same here.
>



-- 
Regards,

Zhi Yong Wu

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [RFC PATCH 4/4] xfs: allow linkat() on O_TMPFILE files

[Christoph Hellwig]

On Thu, Nov 28, 2013 at 10:37:29AM +0800, Zhi Yong Wu wrote:
> On Mon, Nov 25, 2013 at 9:51 PM, Christoph Hellwig <hch@infradead.org> wrote:
> >> -     ASSERT(ip->i_d.di_nlink > 0);
> >> +     if ((VFS_I(ip)->i_nlink == 0) &&
> >> +         !(VFS_I(ip)->i_state & I_LINKABLE))
> >> +             ASSERT(ip->i_d.di_nlink > 0);
> >
> >         ASSERT(ip->i_d.di_nlink > 0 || (VFS_I(ip)->i_state & I_LINKABLE));
> This is wrong, and it should be
>           ASSERT(ip->i_d.di_nlink > 0 || !(VFS_I(ip)->i_state & I_LINKABLE));

Why we want to assrrt that either the link count is bigger than 0,
or that the I_LINKABLE flag is set (for files created using O_TMPFILE)

> >> +
> >> +     if ((VFS_I(sip)->i_nlink == 0) &&
> >> +         (VFS_I(sip)->i_state & I_LINKABLE))
> >> +             tres = &M_RES(mp)->tr_link_tmpfile;
> >> +     else
> >> +             tres = &M_RES(mp)->tr_link;
> >
> > Just check i_nlink, and for consistency it might make sense to just use
> > the xfs_inode one.  The VFS already made sure we don't inodes with
> but struct xfs_inode has no stuff similar to i_nlink....

ip->i_d.di_nlink is the equivalent.

> > I_LINKABLE and a zero link count.
> No, pls see the chunk of code:
> int vfs_link(struct dentry *old_dentry, struct inode *dir, struct
> dentry *new_dentry)
> {
> ...
>     /* Make sure we don't allow creating hardlink to an unlinked file */
>     if (inode->i_nlink == 0 && !(inode->i_state & I_LINKABLE))
>         error =  -ENOENT;

This makes sure we never created a link if the count is zero unless
the I_LINKABLE is set, so we'll never see a zero link count without
I_LINKABLE.

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [RFC PATCH 4/4] xfs: allow linkat() on O_TMPFILE files

[Zhi Yong Wu]

You are right for both following cases, and will fix them, thanks.

On Thu, Nov 28, 2013 at 6:39 PM, Christoph Hellwig <hch@infradead.org> wrote:
> On Thu, Nov 28, 2013 at 10:37:29AM +0800, Zhi Yong Wu wrote:
>> On Mon, Nov 25, 2013 at 9:51 PM, Christoph Hellwig <hch@infradead.org> wrote:
>> >> -     ASSERT(ip->i_d.di_nlink > 0);
>> >> +     if ((VFS_I(ip)->i_nlink == 0) &&
>> >> +         !(VFS_I(ip)->i_state & I_LINKABLE))
>> >> +             ASSERT(ip->i_d.di_nlink > 0);
>> >
>> >         ASSERT(ip->i_d.di_nlink > 0 || (VFS_I(ip)->i_state & I_LINKABLE));
>> This is wrong, and it should be
>>           ASSERT(ip->i_d.di_nlink > 0 || !(VFS_I(ip)->i_state & I_LINKABLE));
>
> Why we want to assrrt that either the link count is bigger than 0,
> or that the I_LINKABLE flag is set (for files created using O_TMPFILE)
>
>> >> +
>> >> +     if ((VFS_I(sip)->i_nlink == 0) &&
>> >> +         (VFS_I(sip)->i_state & I_LINKABLE))
>> >> +             tres = &M_RES(mp)->tr_link_tmpfile;
>> >> +     else
>> >> +             tres = &M_RES(mp)->tr_link;
>> >
>> > Just check i_nlink, and for consistency it might make sense to just use
>> > the xfs_inode one.  The VFS already made sure we don't inodes with
>> but struct xfs_inode has no stuff similar to i_nlink....
>
> ip->i_d.di_nlink is the equivalent.
>
>> > I_LINKABLE and a zero link count.
>> No, pls see the chunk of code:
>> int vfs_link(struct dentry *old_dentry, struct inode *dir, struct
>> dentry *new_dentry)
>> {
>> ...
>>     /* Make sure we don't allow creating hardlink to an unlinked file */
>>     if (inode->i_nlink == 0 && !(inode->i_state & I_LINKABLE))
>>         error =  -ENOENT;
>
> This makes sure we never created a link if the count is zero unless
> the I_LINKABLE is set, so we'll never see a zero link count without
> I_LINKABLE.
>



-- 
Regards,

Zhi Yong Wu

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [RFC PATCH 4/4] xfs: allow linkat() on O_TMPFILE files

[Dave Chinner]

On Mon, Nov 25, 2013 at 07:32:34PM +0800, Zhi Yong Wu wrote:
> From: Zhi Yong Wu <wuzhy@linux.vnet.ibm.com>
> 
> Enable O_TMPFILE support in linkat().
> 
> http://oss.sgi.com/archives/xfs/2013-08/msg00341.html
> 
> Signed-off-by: Zhi Yong Wu <wuzhy@linux.vnet.ibm.com>
> ---
>  fs/xfs/xfs_inode.c      |   23 +++++++++++++++++++++--
>  fs/xfs/xfs_trans_resv.c |   13 +++++++++++++
>  fs/xfs/xfs_trans_resv.h |    2 ++
>  3 files changed, 36 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
> index e1832de..7ab029b 100644
> --- a/fs/xfs/xfs_inode.c
> +++ b/fs/xfs/xfs_inode.c
> @@ -62,6 +62,8 @@ kmem_zone_t *xfs_inode_zone;
>  
>  STATIC int xfs_iflush_int(xfs_inode_t *, xfs_buf_t *);
>  
> +STATIC int xfs_iunlink_remove(xfs_trans_t *, xfs_inode_t *);
> +
>  /*
>   * helper function to extract extent size hint from inode
>   */
> @@ -1119,7 +1121,9 @@ xfs_bumplink(
>  {
>  	xfs_trans_ichgtime(tp, ip, XFS_ICHGTIME_CHG);
>  
> -	ASSERT(ip->i_d.di_nlink > 0);
> +	if ((VFS_I(ip)->i_nlink == 0) &&
> +	    !(VFS_I(ip)->i_state & I_LINKABLE))
> +		ASSERT(ip->i_d.di_nlink > 0);
>  	ip->i_d.di_nlink++;
>  	inc_nlink(VFS_I(ip));
>  	if ((ip->i_d.di_version == 1) &&
> @@ -1473,6 +1477,7 @@ xfs_link(
>  {
>  	xfs_mount_t		*mp = tdp->i_mount;
>  	xfs_trans_t		*tp;
> +	struct xfs_trans_res	*tres;
>  	int			error;
>  	xfs_bmap_free_t         free_list;
>  	xfs_fsblock_t           first_block;
> @@ -1498,7 +1503,14 @@ xfs_link(
>  	tp = xfs_trans_alloc(mp, XFS_TRANS_LINK);
>  	cancel_flags = XFS_TRANS_RELEASE_LOG_RES;
>  	resblks = XFS_LINK_SPACE_RES(mp, target_name->len);
> -	error = xfs_trans_reserve(tp, &M_RES(mp)->tr_link, resblks, 0);
> +
> +	if ((VFS_I(sip)->i_nlink == 0) &&
> +	    (VFS_I(sip)->i_state & I_LINKABLE))
> +		tres = &M_RES(mp)->tr_link_tmpfile;
> +	else
> +		tres = &M_RES(mp)->tr_link;
> +
> +	error = xfs_trans_reserve(tp, tres, resblks, 0);

Uggh. For the small amount of extra space needed for the unlinked
list reservation, I would simply add it to the tr_link reservation
and be done with it. That gets rid of the need for the noise here.

>  	if (error == ENOSPC) {
>  		resblks = 0;
>  		error = xfs_trans_reserve(tp, &M_RES(mp)->tr_link, 0, 0);
> @@ -1530,6 +1542,13 @@ xfs_link(
>  
>  	xfs_bmap_init(&free_list, &first_block);
>  
> +	if ((VFS_I(sip)->i_nlink == 0) &&
> +	    (VFS_I(sip)->i_state & I_LINKABLE)) {
> +		error = xfs_iunlink_remove(tp, sip);
> +		if (error)
> +			goto abort_return;
> +	}
> +
>  	error = xfs_dir_createname(tp, tdp, target_name, sip->i_ino,
>  					&first_block, &free_list, resblks);
>  	if (error)
> diff --git a/fs/xfs/xfs_trans_resv.c b/fs/xfs/xfs_trans_resv.c
> index f1eebe4..7c1234e 100644
> --- a/fs/xfs/xfs_trans_resv.c
> +++ b/fs/xfs/xfs_trans_resv.c
> @@ -228,6 +228,15 @@ xfs_calc_link_reservation(
>  				      XFS_FSB_TO_B(mp, 1))));
>  }
>  
> +STATIC uint
> +xfs_calc_link_tmpfile_reservation(
> +	struct xfs_mount        *mp)
> +{
> +	return xfs_calc_link_reservation(mp) +
> +		xfs_calc_buf_res(2, XFS_FSB_TO_B(mp, 1)) +
> +		xfs_calc_buf_res(2, mp->m_sb.sb_sectsize);
> +}

As per above, fold this into xfs_calc_link_reservation() by adding a:


		+ xfs_calc_iunlink_remove_resv(mp);

to the end of it.  Then you can also modify
xfs_calc_ifree_reservation() to use the same
xfs_calc_iunlink_remove_resv() function as well.

[ And similarly, looking back on the previous patch
xfs_calc_iunlink_add_resv can be used in xfs_calc_remove_reservation
so all the unlinked list manipulations are covered by the same
reservation calculations. ]

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [RFC PATCH 4/4] xfs: allow linkat() on O_TMPFILE files

[Zhi Yong Wu]

On Tue, Nov 26, 2013 at 5:46 AM, Dave Chinner <david@fromorbit.com> wrote:
> On Mon, Nov 25, 2013 at 07:32:34PM +0800, Zhi Yong Wu wrote:
>> From: Zhi Yong Wu <wuzhy@linux.vnet.ibm.com>
>>
>> Enable O_TMPFILE support in linkat().
>>
>> http://oss.sgi.com/archives/xfs/2013-08/msg00341.html
>>
>> Signed-off-by: Zhi Yong Wu <wuzhy@linux.vnet.ibm.com>
>> ---
>>  fs/xfs/xfs_inode.c      |   23 +++++++++++++++++++++--
>>  fs/xfs/xfs_trans_resv.c |   13 +++++++++++++
>>  fs/xfs/xfs_trans_resv.h |    2 ++
>>  3 files changed, 36 insertions(+), 2 deletions(-)
>>
>> diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
>> index e1832de..7ab029b 100644
>> --- a/fs/xfs/xfs_inode.c
>> +++ b/fs/xfs/xfs_inode.c
>> @@ -62,6 +62,8 @@ kmem_zone_t *xfs_inode_zone;
>>
>>  STATIC int xfs_iflush_int(xfs_inode_t *, xfs_buf_t *);
>>
>> +STATIC int xfs_iunlink_remove(xfs_trans_t *, xfs_inode_t *);
>> +
>>  /*
>>   * helper function to extract extent size hint from inode
>>   */
>> @@ -1119,7 +1121,9 @@ xfs_bumplink(
>>  {
>>       xfs_trans_ichgtime(tp, ip, XFS_ICHGTIME_CHG);
>>
>> -     ASSERT(ip->i_d.di_nlink > 0);
>> +     if ((VFS_I(ip)->i_nlink == 0) &&
>> +         !(VFS_I(ip)->i_state & I_LINKABLE))
>> +             ASSERT(ip->i_d.di_nlink > 0);
>>       ip->i_d.di_nlink++;
>>       inc_nlink(VFS_I(ip));
>>       if ((ip->i_d.di_version == 1) &&
>> @@ -1473,6 +1477,7 @@ xfs_link(
>>  {
>>       xfs_mount_t             *mp = tdp->i_mount;
>>       xfs_trans_t             *tp;
>> +     struct xfs_trans_res    *tres;
>>       int                     error;
>>       xfs_bmap_free_t         free_list;
>>       xfs_fsblock_t           first_block;
>> @@ -1498,7 +1503,14 @@ xfs_link(
>>       tp = xfs_trans_alloc(mp, XFS_TRANS_LINK);
>>       cancel_flags = XFS_TRANS_RELEASE_LOG_RES;
>>       resblks = XFS_LINK_SPACE_RES(mp, target_name->len);
>> -     error = xfs_trans_reserve(tp, &M_RES(mp)->tr_link, resblks, 0);
>> +
>> +     if ((VFS_I(sip)->i_nlink == 0) &&
>> +         (VFS_I(sip)->i_state & I_LINKABLE))
>> +             tres = &M_RES(mp)->tr_link_tmpfile;
>> +     else
>> +             tres = &M_RES(mp)->tr_link;
>> +
>> +     error = xfs_trans_reserve(tp, tres, resblks, 0);
>
> Uggh. For the small amount of extra space needed for the unlinked
> list reservation, I would simply add it to the tr_link reservation
> and be done with it. That gets rid of the need for the noise here.
will apply this.
>
>>       if (error == ENOSPC) {
>>               resblks = 0;
>>               error = xfs_trans_reserve(tp, &M_RES(mp)->tr_link, 0, 0);
>> @@ -1530,6 +1542,13 @@ xfs_link(
>>
>>       xfs_bmap_init(&free_list, &first_block);
>>
>> +     if ((VFS_I(sip)->i_nlink == 0) &&
>> +         (VFS_I(sip)->i_state & I_LINKABLE)) {
>> +             error = xfs_iunlink_remove(tp, sip);
>> +             if (error)
>> +                     goto abort_return;
>> +     }
>> +
>>       error = xfs_dir_createname(tp, tdp, target_name, sip->i_ino,
>>                                       &first_block, &free_list, resblks);
>>       if (error)
>> diff --git a/fs/xfs/xfs_trans_resv.c b/fs/xfs/xfs_trans_resv.c
>> index f1eebe4..7c1234e 100644
>> --- a/fs/xfs/xfs_trans_resv.c
>> +++ b/fs/xfs/xfs_trans_resv.c
>> @@ -228,6 +228,15 @@ xfs_calc_link_reservation(
>>                                     XFS_FSB_TO_B(mp, 1))));
>>  }
>>
>> +STATIC uint
>> +xfs_calc_link_tmpfile_reservation(
>> +     struct xfs_mount        *mp)
>> +{
>> +     return xfs_calc_link_reservation(mp) +
>> +             xfs_calc_buf_res(2, XFS_FSB_TO_B(mp, 1)) +
>> +             xfs_calc_buf_res(2, mp->m_sb.sb_sectsize);
>> +}
>
> As per above, fold this into xfs_calc_link_reservation() by adding a:
>
>
>                 + xfs_calc_iunlink_remove_resv(mp);
>
> to the end of it.  Then you can also modify
> xfs_calc_ifree_reservation() to use the same
> xfs_calc_iunlink_remove_resv() function as well.
>
> [ And similarly, looking back on the previous patch
> xfs_calc_iunlink_add_resv can be used in xfs_calc_remove_reservation
> so all the unlinked list manipulations are covered by the same
> reservation calculations. ]
Good suggestion, will try to apply them in next version, thanks.
>
> Cheers,
>
> Dave.
> --
> Dave Chinner
> david@fromorbit.com



-- 
Regards,

Zhi Yong Wu

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [RFC PATCH 4/4] xfs: allow linkat() on O_TMPFILE files

[Zhi Yong Wu]

On Tue, Nov 26, 2013 at 5:46 AM, Dave Chinner <david@fromorbit.com> wrote:
> On Mon, Nov 25, 2013 at 07:32:34PM +0800, Zhi Yong Wu wrote:
>> From: Zhi Yong Wu <wuzhy@linux.vnet.ibm.com>
>>
>> Enable O_TMPFILE support in linkat().
>>
>> http://oss.sgi.com/archives/xfs/2013-08/msg00341.html
>>
>> Signed-off-by: Zhi Yong Wu <wuzhy@linux.vnet.ibm.com>
>> ---
>>  fs/xfs/xfs_inode.c      |   23 +++++++++++++++++++++--
>>  fs/xfs/xfs_trans_resv.c |   13 +++++++++++++
>>  fs/xfs/xfs_trans_resv.h |    2 ++
>>  3 files changed, 36 insertions(+), 2 deletions(-)
>>
>> diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
>> index e1832de..7ab029b 100644
>> --- a/fs/xfs/xfs_inode.c
>> +++ b/fs/xfs/xfs_inode.c
>> @@ -62,6 +62,8 @@ kmem_zone_t *xfs_inode_zone;
>>
>>  STATIC int xfs_iflush_int(xfs_inode_t *, xfs_buf_t *);
>>
>> +STATIC int xfs_iunlink_remove(xfs_trans_t *, xfs_inode_t *);
>> +
>>  /*
>>   * helper function to extract extent size hint from inode
>>   */
>> @@ -1119,7 +1121,9 @@ xfs_bumplink(
>>  {
>>       xfs_trans_ichgtime(tp, ip, XFS_ICHGTIME_CHG);
>>
>> -     ASSERT(ip->i_d.di_nlink > 0);
>> +     if ((VFS_I(ip)->i_nlink == 0) &&
>> +         !(VFS_I(ip)->i_state & I_LINKABLE))
>> +             ASSERT(ip->i_d.di_nlink > 0);
>>       ip->i_d.di_nlink++;
>>       inc_nlink(VFS_I(ip));
>>       if ((ip->i_d.di_version == 1) &&
>> @@ -1473,6 +1477,7 @@ xfs_link(
>>  {
>>       xfs_mount_t             *mp = tdp->i_mount;
>>       xfs_trans_t             *tp;
>> +     struct xfs_trans_res    *tres;
>>       int                     error;
>>       xfs_bmap_free_t         free_list;
>>       xfs_fsblock_t           first_block;
>> @@ -1498,7 +1503,14 @@ xfs_link(
>>       tp = xfs_trans_alloc(mp, XFS_TRANS_LINK);
>>       cancel_flags = XFS_TRANS_RELEASE_LOG_RES;
>>       resblks = XFS_LINK_SPACE_RES(mp, target_name->len);
>> -     error = xfs_trans_reserve(tp, &M_RES(mp)->tr_link, resblks, 0);
>> +
>> +     if ((VFS_I(sip)->i_nlink == 0) &&
>> +         (VFS_I(sip)->i_state & I_LINKABLE))
>> +             tres = &M_RES(mp)->tr_link_tmpfile;
>> +     else
>> +             tres = &M_RES(mp)->tr_link;
>> +
>> +     error = xfs_trans_reserve(tp, tres, resblks, 0);
>
> Uggh. For the small amount of extra space needed for the unlinked
> list reservation, I would simply add it to the tr_link reservation
> and be done with it. That gets rid of the need for the noise here.
>
>
> As per above, fold this into xfs_calc_link_reservation() by adding a:
>
>
>                 + xfs_calc_iunlink_remove_resv(mp);
This way seems to not work...
In xfs_calc_link_reservation(), How to determine if it is for one
regular file or O_TMPFILE file? since this functions is only passed in
mp?

>
> to the end of it.  Then you can also modify
> xfs_calc_ifree_reservation() to use the same
> xfs_calc_iunlink_remove_resv() function as well.
>
> [ And similarly, looking back on the previous patch
> xfs_calc_iunlink_add_resv can be used in xfs_calc_remove_reservation
> so all the unlinked list manipulations are covered by the same
> reservation calculations. ]
>
> Cheers,
>
> Dave.
> --
> Dave Chinner
> david@fromorbit.com



-- 
Regards,

Zhi Yong Wu

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [RFC PATCH 4/4] xfs: allow linkat() on O_TMPFILE files

[Christoph Hellwig]

On Fri, Dec 13, 2013 at 07:34:43PM +0800, Zhi Yong Wu wrote:
> > Uggh. For the small amount of extra space needed for the unlinked
> > list reservation, I would simply add it to the tr_link reservation
> > and be done with it. That gets rid of the need for the noise here.
> >
> >
> > As per above, fold this into xfs_calc_link_reservation() by adding a:
> >
> >
> >                 + xfs_calc_iunlink_remove_resv(mp);
> This way seems to not work...
> In xfs_calc_link_reservation(), How to determine if it is for one
> regular file or O_TMPFILE file? since this functions is only passed in
> mp?

The way I understand Dave above is that you should always reserve
the higher amount.

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [RFC PATCH 0/4] xfs: add O_TMPFILE support

[Christoph Hellwig]

On Mon, Nov 25, 2013 at 07:32:30PM +0800, Zhi Yong Wu wrote:
> From: Zhi Yong Wu <wuzhy@linux.vnet.ibm.com>
> 
> HI, folks
> 
> The patchset is trying to add O_TMPFILE support to xfs. To make sure
> that it is going ahead in the right direction, although it is only one
> code draft, and hasn't strictly been tested, it is still post out ASAP.

Btw, for the real submission we need proper xfstests coverage, I can't
really believe how the other filesystems added it without QA coverage :(

Besides the obvious test programs there's a reproducer for a a simple
bug already in commit dda5690defe4af62ee120f055e98e40d97e4c760 in the
kernel tree.

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [RFC PATCH 0/4] xfs: add O_TMPFILE support

[Zhi Yong Wu]

On Thu, Nov 28, 2013 at 6:35 PM, Christoph Hellwig <hch@infradead.org> wrote:
> On Mon, Nov 25, 2013 at 07:32:30PM +0800, Zhi Yong Wu wrote:
>> From: Zhi Yong Wu <wuzhy@linux.vnet.ibm.com>
>>
>> HI, folks
>>
>> The patchset is trying to add O_TMPFILE support to xfs. To make sure
>> that it is going ahead in the right direction, although it is only one
>> code draft, and hasn't strictly been tested, it is still post out ASAP.
>
> Btw, for the real submission we need proper xfstests coverage, I can't
ok, i will try to run xfstests before real submission is done.
> really believe how the other filesystems added it without QA coverage :(
heh, i guess that you should ask those fs maintainers this question.
>
> Besides the obvious test programs there's a reproducer for a a simple
> bug already in commit dda5690defe4af62ee120f055e98e40d97e4c760 in the
> kernel tree.
ok, will test if this bug is triggered.
>



-- 
Regards,

Zhi Yong Wu

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [RFC PATCH 0/4] xfs: add O_TMPFILE support

[Christoph Hellwig]

> ok, i will try to run xfstests before real submission is done.

There's no coverage of O_TMPFILE yet, but we do need it if we want
to support the feature.  

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs