xfs: tmpfile fixes
Subsystems (2)
v3 cover xfs: tmpfile fixes
Hi all,
Here's a v3 series for the patches previously posted here:
http://oss.sgi.com/archives/xfs/2014-04/msg00181.html
Patches 1 and 2 are just a split-up of the v1 patch:
http://oss.sgi.com/archives/xfs/2014-04/msg00149.html
Note the v1 patch has a reviewed-by, so feel free to drop 1 and 2 here in favor of that version. Patches 3 and 4 are a couple cleanups in the xfs_create() path.
Setting the default ACL is dropped until it is determined how that should be handled for tmpfile(). This means the xfs_iops.c refactor bits have been dropped as well.
Brian
v3:
- Split up the deadlock fix and inode security initialization into separate patches. [hch]
- Dropped the default ACL and associated refactor bits for now. [hch]
- Split the xfs_create() tres and xfs_create_tmpfile() cleanups into separate patches. [hch]
v2:
- Added a generic create helper to inherit the security/acl init. code in the tmpfile path.
- Added patch 2 to fold xfs_create_tmpfile() into xfs_create().
Brian Foster (4):
xfs: fix tmpfile/selinux ilock deadlock
xfs: initialize inode security on tmpfile creation
xfs: replace on-stack xfs_trans_res with pointer in xfs_create()
xfs: fold xfs_create_tmpfile() into xfs_create()
fs/xfs/xfs_inode.c | 193 +++++++++++++---------------------------------------- fs/xfs/xfs_inode.h | 2 - fs/xfs/xfs_iops.c | 20 +++++- fs/xfs/xfs_trace.h | 7 +- 4 files changed, 69 insertions(+), 153 deletions(-)
Signature · 6 lines
1.8.3.1 _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs
PATCH v3 1/4 xfs: fix tmpfile/selinux ilock deadlock
xfstests generic/004 reproduces an ilock deadlock using the tmpfile interface when selinux is enabled. This occurs because xfs_create_tmpfile() takes the ilock and then calls d_tmpfile(). The latter eventually calls into xfs_xattr_get() which attempts to get the lock again. E.g.:
xfs_io D ffffffff81c134c0 4096 3561 3560 0x00000080ffff8801176a1a68 0000000000000046 ffff8800b401b540 ffff8801176a1fd8 00000000001d5800 00000000001d5800 ffff8800b401b540 ffff8800b401b540 ffff8800b73a6bd0 fffffffeffffffff ffff8800b73a6bd8 ffff8800b5ddb480
Call Trace: [<ffffffff8177f969>] schedule+0x29/0x70 [<ffffffff81783a65>] rwsem_down_read_failed+0xc5/0x120 [<ffffffffa05aa97f>] ? xfs_ilock_attr_map_shared+0x1f/0x50 [xfs] [<ffffffff813b3434>] call_rwsem_down_read_failed+0x14/0x30 [<ffffffff810ed179>] ? down_read_nested+0x89/0xa0 [<ffffffffa05aa7f2>] ? xfs_ilock+0x122/0x250 [xfs] [<ffffffffa05aa7f2>] xfs_ilock+0x122/0x250 [xfs] [<ffffffffa05aa97f>] xfs_ilock_attr_map_shared+0x1f/0x50 [xfs] [<ffffffffa05701d0>] xfs_attr_get+0x90/0xe0 [xfs] [<ffffffffa0565e07>] xfs_xattr_get+0x37/0x50 [xfs] [<ffffffff8124842f>] generic_getxattr+0x4f/0x70 [<ffffffff8133fd9e>] inode_doinit_with_dentry+0x1ae/0x650 [<ffffffff81340e0c>] selinux_d_instantiate+0x1c/0x20 [<ffffffff813351bb>] security_d_instantiate+0x1b/0x30 [<ffffffff81237db0>] d_instantiate+0x50/0x70 [<ffffffff81237e85>] d_tmpfile+0xb5/0xc0 [<ffffffffa05add02>] xfs_create_tmpfile+0x362/0x410 [xfs] [<ffffffffa0559ac8>] xfs_vn_tmpfile+0x18/0x20 [xfs] [<ffffffff81230388>] path_openat+0x228/0x6a0 [<ffffffff810230f9>] ? sched_clock+0x9/0x10 [<ffffffff8105a427>] ? kvm_clock_read+0x27/0x40 [<ffffffff8124054f>] ? __alloc_fd+0xaf/0x1f0 [<ffffffff8123101a>] do_filp_open+0x3a/0x90 [<ffffffff817845e7>] ? _raw_spin_unlock+0x27/0x40 [<ffffffff8124054f>] ? __alloc_fd+0xaf/0x1f0 [<ffffffff8121e3ce>] do_sys_open+0x12e/0x210 [<ffffffff8121e4ce>] SyS_open+0x1e/0x20 [<ffffffff8178eda9>] system_call_fastpath+0x16/0x1b
Pull the d_tmpfile() call up into xfs_vn_tmpfile() after the transaction has been committed and the inode unlocked. This pattern is consistent with other dcache callers (e.g., d_instantiate()) in xfs_iops.c.
--- fs/xfs/xfs_inode.c | 6 +++--- fs/xfs/xfs_inode.h | 4 ++-- fs/xfs/xfs_iops.c | 14 +++++++++++--- 3 files changed, 16 insertions(+), 8 deletions(-)
fs/xfs/xfs_inode.c +3 -3
@@ -1333,8 +1333,8 @@ xfs_create(int xfs_create_tmpfile (struct xfs_inode *dp, -struct dentry *dentry, -umode_t mode) +umode_t mode, +struct xfs_inode **ipp) {struct xfs_mount *mp = dp->i_mount;struct xfs_inode *ip =NULL ;
@@ -1402,7 +1402,6 @@ xfs_create_tmpfile(xfs_qm_vop_create_dqattach (tp, ip, udqp, gdqp, pdqp); ip->i_d.di_nlink--; -d_tmpfile (dentry,VFS_I (ip)); error =xfs_iunlink (tp, ip);if (error) goto out_trans_abort;
@@ -1415,6 +1414,7 @@ xfs_create_tmpfile(xfs_qm_dqrele (gdqp);xfs_qm_dqrele (pdqp); + *ipp = ip;return 0 ; out_trans_abort:
fs/xfs/xfs_inode.h +2 -2
@@ -333,8 +333,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_inode *dp,struct dentry *dentry, -umode_t mode); +int xfs_create_tmpfile (struct xfs_inode *dp,umode_t mode, +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,
fs/xfs/xfs_iops.c +11 -3
@@ -1053,11 +1053,19 @@ xfs_vn_tmpfile(struct dentry *dentry, umode_t mode) { - int error; + int error; +struct xfs_inode *ip; +struct inode *inode; - error =xfs_create_tmpfile (XFS_I (dir), dentry, mode); + error =xfs_create_tmpfile (XFS_I (dir), mode, &ip); +if (unlikely (error)) +return -error; -return -error; + inode =VFS_I (ip); + +d_tmpfile (dentry, inode); + +return 0 ; } static const struct inode_operations xfs_inode_operations = {
Signature · 6 lines
1.8.3.1 _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs
Re: [PATCH v3 1/4] xfs: fix tmpfile/selinux ilock deadlock
Looks good,
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
PATCH v3 2/4 xfs: initialize inode security on tmpfile creation
In reply to Brian FosterInitialize security for inodes allocated via the tmpfile interface. This ensures security is initialized if the inode is subsequently linked back into the namespace.
--- fs/xfs/xfs_iops.c | 6 ++++++ 1 file changed, 6 insertions(+)
fs/xfs/xfs_iops.c +6 -0
@@ -1063,6 +1063,12 @@ xfs_vn_tmpfile( inode =VFS_I (ip); + error =xfs_init_security (inode, dir, &dentry->d_name); +if (unlikely (error)) { +iput (inode); +return -error; + } +d_tmpfile (dentry, inode);return 0 ;
Signature · 6 lines
1.8.3.1 _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs
Re: [PATCH v3 2/4] xfs: initialize inode security on tmpfile creation
fs/xfs/xfs_iops.c xfs_vn_tmpfile( · line 1072
Quoted 8 lines · Brian Foster
fs/xfs/xfs_iops.c +6
-0+ error =xfs_init_security (inode, dir, &dentry->d_name); +if (unlikely (error)) { +iput (inode); +return -error; + } +d_tmpfile (dentry, inode);
I'd really love to hear from the LSM people who they plan to deal withO_TMPFILE inodes. But given that this seems to fix a real life bug
let's go with it for now.
Re: [PATCH v3 2/4] xfs: initialize inode security on tmpfile creation
Quoted 13 lines · Christoph Hellwig
fs/xfs/xfs_iops.c xfs_vn_tmpfile( · line 1072
Quoted 8 lines · Brian Foster
fs/xfs/xfs_iops.c +6
-0+ error =xfs_init_security (inode, dir, &dentry->d_name); +if (unlikely (error)) { +iput (inode); +return -error; + } +d_tmpfile (dentry, inode);I'd really love to hear from the LSM people who they plan to deal with
O_TMPFILEinodes. But given that this seems to fix a real life bug
let's go with it for now.
Is there a reason that xfs_init_security() isn't called from the inode allocation function (e.g. xfs_ialloc), as in ext4 (__ext4_new_inode calls ext4_init_security and also calls ext4_init_acl)? That would have ensured that tmpfile inodes would have been labeled without requiring a separate change and more generally ensures complete coverage for all inodes.
For SELinux, we need the tmpfile inodes to be labeled at creation time, not just if linked into the namespace, since they may be shared via local socket IPC or inherited across a label-changing exec and since we revalidate access on transfer or use.
Labeling based on the provided directory could be a bit random, although it will work out with current policy if the provided directory corresponds to existing tmpfile locations (e.g. /tmp, /var/tmp) and therefore already has a label associated with temporary files. Otherwise we might want some indication that it is a tmpfile passed into security_inode_init_security() so that we can always select a stable label irrespective of the directory.
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
Re: [PATCH v3 2/4] xfs: initialize inode security on tmpfile creation
Quoted 26 lines · Stephen Smalley
Quoted 13 lines · Christoph Hellwig
fs/xfs/xfs_iops.c xfs_vn_tmpfile( · line 1072
Quoted 8 lines · Brian Foster
fs/xfs/xfs_iops.c +6
-0+ error =xfs_init_security (inode, dir, &dentry->d_name); +if (unlikely (error)) { +iput (inode); +return -error; + } +d_tmpfile (dentry, inode);I'd really love to hear from the LSM people who they plan to deal with
O_TMPFILEinodes. But given that this seems to fix a real life bug
let's go with it for now.Is there a reason that
xfs_init_security()isn't called from the inode allocation function (e.g.xfs_ialloc), as in ext4 (__ext4_new_inodecallsext4_init_securityand also callsext4_init_acl)? That would have ensured that tmpfile inodes would have been labeled without requiring a separate change and more generally ensures complete coverage for all inodes.For SELinux, we need the tmpfile inodes to be labeled at creation time, not just if linked into the namespace, since they may be shared via local socket IPC or inherited across a label-changing exec and since we revalidate access on transfer or use.
Quoted 7 lines
Labeling based on the provided directory could be a bit random, although it will work out with current policy if the provided directory corresponds to existing tmpfile locations (e.g. /tmp,
/var/tmp) and therefore already has a label associated with temporary files. Otherwise we might want some indication that it is a tmpfile passed intosecurity_inode_init_security()so that we can always select a stable label irrespective of the directory.
Hmm...wondering if we can use the qstr as a distinguisher; pass NULL for tmpfile and not for others as in ext4?
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
Re: [PATCH v3 2/4] xfs: initialize inode security on tmpfile creation
In reply to Stephen SmalleyQuoted 5 lines · Stephen Smalley
Is there a reason that
xfs_init_security()isn't called from the inode allocation function (e.g.xfs_ialloc), as in ext4 (__ext4_new_inodecallsext4_init_securityand also callsext4_init_acl)? That would have ensured that tmpfile inodes would have been labeled without requiring a separate change and more generally ensures complete coverage for all inodes.
Really just code structuring - we don't like callouts to high level VFS functions from deep down in the guts of the filesystem.
Quoted 5 lines
For SELinux, we need the tmpfile inodes to be labeled at creation time, not just if linked into the namespace, since they may be shared via local socket IPC or inherited across a label-changing exec and since we revalidate access on transfer or use.
Quoted 7 lines
Labeling based on the provided directory could be a bit random, although it will work out with current policy if the provided directory corresponds to existing tmpfile locations (e.g. /tmp,
/var/tmp) and therefore already has a label associated with temporary files. Otherwise we might want some indication that it is a tmpfile passed intosecurity_inode_init_security()so that we can always select a stable label irrespective of the directory.
Just check for I_LINKABLE in i_flags.
Re: [PATCH v3 2/4] xfs: initialize inode security on tmpfile creation
Quoted 23 lines · Christoph Hellwig
Quoted 5 lines · Stephen Smalley
Is there a reason that
xfs_init_security()isn't called from the inode allocation function (e.g.xfs_ialloc), as in ext4 (__ext4_new_inodecallsext4_init_securityand also callsext4_init_acl)? That would have ensured that tmpfile inodes would have been labeled without requiring a separate change and more generally ensures complete coverage for all inodes.Really just code structuring - we don't like callouts to high level VFS functions from deep down in the guts of the filesystem.
Quoted 5 lines
For SELinux, we need the tmpfile inodes to be labeled at creation time, not just if linked into the namespace, since they may be shared via local socket IPC or inherited across a label-changing exec and since we revalidate access on transfer or use.
Quoted 7 lines
Labeling based on the provided directory could be a bit random, although it will work out with current policy if the provided directory corresponds to existing tmpfile locations (e.g. /tmp,
/var/tmp) and therefore already has a label associated with temporary files. Otherwise we might want some indication that it is a tmpfile passed intosecurity_inode_init_security()so that we can always select a stable label irrespective of the directory.
Quoted 1 line
Just check for
I_LINKABLEini_flags.
Thanks, that should allow us to handle it cleanly in the security modules!
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
Re: [PATCH v3 2/4] xfs: initialize inode security on tmpfile creation
Quoted 24 lines · Stephen Smalley
Quoted 23 lines · Christoph Hellwig
Quoted 5 lines · Stephen Smalley
Is there a reason that
xfs_init_security()isn't called from the inode allocation function (e.g.xfs_ialloc), as in ext4 (__ext4_new_inodecallsext4_init_securityand also callsext4_init_acl)? That would have ensured that tmpfile inodes would have been labeled without requiring a separate change and more generally ensures complete coverage for all inodes.Really just code structuring - we don't like callouts to high level VFS functions from deep down in the guts of the filesystem.
Quoted 12 lines
For SELinux, we need the tmpfile inodes to be labeled at creation time, not just if linked into the namespace, since they may be shared via local socket IPC or inherited across a label-changing exec and since we revalidate access on transfer or use.
Labeling based on the provided directory could be a bit random, although it will work out with current policy if the provided directory corresponds to existing tmpfile locations (e.g. /tmp,
/var/tmp) and therefore already has a label associated with temporary files. Otherwise we might want some indication that it is a tmpfile passed intosecurity_inode_init_security()so that we can always select a stable label irrespective of the directory.
Quoted 3 lines
Quoted 1 line
Just check for
I_LINKABLEini_flags.Thanks, that should allow us to handle it cleanly in the security modules!
Maybe I spoke too soon. IIUC, I_LINKABLE doesn't necessarily distinguish tmpfiles from other files, as some tmpfiles may be linkable and others not. But what we want is a way to identify all tmpfiles when security_inode_init_security() is called if we are going to label them independently of the provided dir.
Also, in that situation, we would need to likewise distinguish them during the create-time checking, i.e. when security_inode_create() is called (from may_o_create), as we have to determine the label that will be applied at that point for permission checking. And there we do not have the inode yet so we do not even have I_LINKABLE as a distinguisher.
So I think we need __O_TMPFILE or similar flag passed down to may_o_create() -> security_inode_create() and to security_inode_init_security() if we are going to label these files independently of the provided directory.
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
Re: [PATCH v3 2/4] xfs: initialize inode security on tmpfile creation
Quoted 5 lines · Stephen Smalley
Maybe I spoke too soon. IIUC,
I_LINKABLEdoesn't necessarily distinguish tmpfiles from other files, as some tmpfiles may be linkable and others not. But what we want is a way to identify all tmpfiles whensecurity_inode_init_security()is called if we are going to label them independently of the provided dir.
Oh, right. If O_EXCL is specified (another annoying overload of the flag..) the tmpfile can't ever be linked back into the filesystem and thus doesn't have I_LINKABLE set.
I guess the best way to fix this is using the magic qstr you suggested before. That means security_inode_init_security would need to be called after d_tmpfile, which most filesystems don't do right now.
Re: [PATCH v3 2/4] xfs: initialize inode security on tmpfile creation
Quoted 7 lines · Christoph Hellwig
Quoted 5 lines · Stephen Smalley
Maybe I spoke too soon. IIUC,
I_LINKABLEdoesn't necessarily distinguish tmpfiles from other files, as some tmpfiles may be linkable and others not. But what we want is a way to identify all tmpfiles whensecurity_inode_init_security()is called if we are going to label them independently of the provided dir.
Quoted 7 lines
Oh, right. If
O_EXCLis specified (another annoying overload of the flag..) the tmpfile can't ever be linked back into the filesystem and thus doesn't haveI_LINKABLEset.I guess the best way to fix this is using the magic qstr you suggested before. That means
security_inode_init_securitywould need to be called afterd_tmpfile, which most filesystems don't do right now.
I think one could just pass NULL for the qstr as an indicator, which ext4 already does, so it doesn't require moving after d_tmpfile) IIUC. However, that doesn't solve the problem for security_inode_create(), which also needs to know it is dealing with a tmpfile. So we might want to just pass an explicit flag to both.
PATCH v3 3/4 xfs: replace on-stack xfs_trans_res with pointer in xfs_create()
In reply to Brian FosterThere's no need to store a full struct xfs_trans_res on the stack in xfs_create() and copy the fields. Use a pointer to the appropriate structures embedded in the xfs_mount.
--- fs/xfs/xfs_inode.c | 15 ++++++--------- 1 file changed, 6 insertions(+), 9 deletions(-)
fs/xfs/xfs_inode.c +6 -9
@@ -1158,7 +1158,7 @@ xfs_create(struct xfs_dquot *udqp =NULL ;struct xfs_dquot *gdqp =NULL ;struct xfs_dquot *pdqp =NULL ; -struct xfs_trans_res tres; +struct xfs_trans_res *tres;uint resblks;trace_xfs_create (dp, name);
@@ -1181,13 +1181,11 @@ xfs_create(if (is_dir) { rdev =0 ; resblks =XFS_MKDIR_SPACE_RES (mp, name->len); - tres.tr_logres =M_RES (mp)->tr_mkdir.tr_logres; - tres.tr_logcount =XFS_MKDIR_LOG_COUNT ; + tres = &M_RES (mp)->tr_mkdir; tp =xfs_trans_alloc (mp,XFS_TRANS_MKDIR ); }else { resblks =XFS_CREATE_SPACE_RES (mp, name->len); - tres.tr_logres =M_RES (mp)->tr_create.tr_logres; - tres.tr_logcount =XFS_CREATE_LOG_COUNT ; + tres = &M_RES (mp)->tr_create; tp =xfs_trans_alloc (mp,XFS_TRANS_CREATE ); }
@@ -1199,17 +1197,16 @@ xfs_create(* the case we'll drop the one we have and get a more * appropriate transaction later. */ - tres.tr_logflags =XFS_TRANS_PERM_LOG_RES ; - error =xfs_trans_reserve (tp, &tres, resblks,0 ); + 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 ); + 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 ); + error =xfs_trans_reserve (tp, tres,0 ,0 ); }if (error) { cancel_flags =0 ;
Signature · 6 lines
1.8.3.1 _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs
Re: [PATCH v3 3/4] xfs: replace on-stack xfs_trans_res with pointer in xfs_create()
Quoted 5 lines · Brian Foster
There's no need to store a full struct
xfs_trans_reson the stack inxfs_create()and copy the fields. Use a pointer to the appropriate structures embedded in thexfs_mount.Signed-off-by Brian Foster <[email protected]>
Looks good,
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
PATCH v3 4/4 xfs: fold xfs_create_tmpfile() into xfs_create()
In reply to Brian Fosterxfs_create_tmpfile() duplicates most of xfs_create() with the exception of a couple minor differences. We use a unique transaction reservation and place the allocated inode on the unlinked list rather than create a directory entry for it.
Fold xfs_create_tmpfile() into xfs_create() to reduce some of this duplication. The name parameter that represents the name of the directory entry to create is now conditional and its existence dictates whether a directory entry is created for the inode or not.
--- fs/xfs/xfs_inode.c | 178 +++++++++++++---------------------------------------- fs/xfs/xfs_inode.h | 2 - fs/xfs/xfs_iops.c | 2 +- fs/xfs/xfs_trace.h | 7 ++- 4 files changed, 47 insertions(+), 142 deletions(-)
fs/xfs/xfs_inode.c +42 -136
@@ -1183,10 +1183,14 @@ xfs_create( resblks =XFS_MKDIR_SPACE_RES (mp, name->len); tres = &M_RES (mp)->tr_mkdir; tp =xfs_trans_alloc (mp,XFS_TRANS_MKDIR ); - }else { + }else if (name) { resblks =XFS_CREATE_SPACE_RES (mp, name->len); tres = &M_RES (mp)->tr_create; tp =xfs_trans_alloc (mp,XFS_TRANS_CREATE ); + }else { + resblks =XFS_IALLOC_SPACE_RES (mp); + tres = &M_RES (mp)->tr_create_tmpfile; + tp =xfs_trans_alloc (mp,XFS_TRANS_CREATE_TMPFILE ); } cancel_flags =XFS_TRANS_RELEASE_LOG_RES ;
@@ -1213,9 +1217,6 @@ xfs_create( goto out_trans_cancel; } -xfs_ilock (dp,XFS_ILOCK_EXCL |XFS_ILOCK_PARENT ); - unlock_dp_on_error = true; -xfs_bmap_init (&free_list, &first_block);/*
@@ -1226,9 +1227,14 @@ xfs_create(if (error) goto out_trans_cancel; - error =xfs_dir_canenter (tp, dp, name, resblks); -if (error) - goto out_trans_cancel; +if (name) { +xfs_ilock (dp,XFS_ILOCK_EXCL |XFS_ILOCK_PARENT ); + unlock_dp_on_error = true; + + error =xfs_dir_canenter (tp, dp, name, resblks); +if (error) + goto out_trans_cancel; + }/* * A newly created regular or special file just has one directory
@@ -1243,34 +1249,41 @@ xfs_create( goto out_trans_abort; } -/* -* Now we join the directory inode to the transaction. We do not do it -* earlier because xfs_dir_ialloc might commit the previous transaction -* (and release all the locks). An error from here on will result in -* the transaction cancel unlocking dp so don't do it explicitly in the -* error path. -*/ -xfs_trans_ijoin (tp, dp,XFS_ILOCK_EXCL ); - unlock_dp_on_error = false; +if (name) { +/* +* Now we join the directory inode to the transaction. We do not +* do it earlier because xfs_dir_ialloc might commit the +* previous transaction (and release all the locks). An error +* from here on will result in the transaction cancel unlocking +* dp so don't do it explicitly in the error path. +*/ +xfs_trans_ijoin (tp, dp,XFS_ILOCK_EXCL ); + unlock_dp_on_error = false; - error =xfs_dir_createname (tp, dp, name, ip->i_ino, + error =xfs_dir_createname (tp, dp, name, ip->i_ino, &first_block, &free_list, resblks ? resblks -XFS_IALLOC_SPACE_RES (mp) :0 ); -if (error) { -ASSERT (error !=ENOSPC ); - goto out_trans_abort; - } -xfs_trans_ichgtime (tp, dp,XFS_ICHGTIME_MOD |XFS_ICHGTIME_CHG ); -xfs_trans_log_inode (tp, dp,XFS_ILOG_CORE ); +if (error) { +ASSERT (error !=ENOSPC ); + goto out_trans_abort; + } +xfs_trans_ichgtime (tp, dp,XFS_ICHGTIME_MOD |XFS_ICHGTIME_CHG ); +xfs_trans_log_inode (tp, dp,XFS_ILOG_CORE ); -if (is_dir) { - error =xfs_dir_init (tp, ip, dp); -if (error) - goto out_bmap_cancel; +if (is_dir) { + error =xfs_dir_init (tp, ip, dp); +if (error) + goto out_bmap_cancel; - error =xfs_bumplink (tp, dp); + error =xfs_bumplink (tp, dp); +if (error) + goto out_bmap_cancel; + } + }else { + ip->i_d.di_nlink--; + error =xfs_iunlink (tp, ip);if (error) - goto out_bmap_cancel; + goto out_trans_abort; }/*
@@ -1328,113 +1341,6 @@ xfs_create( } int -xfs_create_tmpfile ( -struct xfs_inode *dp, -umode_t mode, -struct xfs_inode **ipp) -{ -struct xfs_mount *mp = dp->i_mount; -struct xfs_inode *ip =NULL ; -struct xfs_trans *tp =NULL ; -int error; -uint cancel_flags =XFS_TRANS_RELEASE_LOG_RES ; -prid_t prid; -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 ); - - prid =xfs_get_initial_prid (dp); - -/* -* Make sure that we have allocated dquot(s) on disk. -*/ - error =xfs_qm_vop_dqalloc (dp,xfs_kuid_to_uid (current_fsuid ()), -xfs_kgid_to_gid (current_fsgid ()), prid, -XFS_QMOPT_QUOTALL |XFS_QMOPT_INHERIT , - &udqp, &gdqp, &pdqp); -if (error) -return error; - - resblks =XFS_IALLOC_SPACE_RES (mp); - tp =xfs_trans_alloc (mp,XFS_TRANS_CREATE_TMPFILE ); - - tres = &M_RES (mp)->tr_create_tmpfile; - 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; - } - - error =xfs_trans_reserve_quota (tp, mp, udqp, gdqp, - pdqp, resblks,1 ,0 ); -if (error) - goto out_trans_cancel; - - error =xfs_dir_ialloc (&tp, dp, mode,1 ,0 , - prid, resblks >0 , &ip,NULL ); -if (error) { -if (error ==ENOSPC ) - goto out_trans_cancel; - goto out_trans_abort; - } - -if (mp->m_flags &XFS_MOUNT_WSYNC ) -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); - - ip->i_d.di_nlink--; - 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,
fs/xfs/xfs_inode.h +0 -2
@@ -333,8 +333,6 @@ 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_inode *dp,umode_t mode, -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,
fs/xfs/xfs_iops.c +1 -1
@@ -1057,7 +1057,7 @@ xfs_vn_tmpfile(struct xfs_inode *ip;struct inode *inode; - error =xfs_create_tmpfile (XFS_I (dir), mode, &ip); + error =xfs_create (XFS_I (dir),NULL , mode,0 , &ip);if (unlikely (error))return -error;
fs/xfs/xfs_trace.h +4 -3
@@ -691,13 +691,14 @@ DECLARE_EVENT_CLASS(xfs_namespace_class, __field(dev_t, dev) __field(xfs_ino_t, dp_ino) __field(int, namelen) -__dynamic_array (char, name, name->len) +__dynamic_array (char, name, name ? name->len :0 ) ),TP_fast_assign ( __entry->dev = VFS_I(dp)->i_sb->s_dev; __entry->dp_ino = dp->i_ino; - __entry->namelen = name->len; -memcpy (__get_str (name), name->name, name->len); + __entry->namelen = name ? name->len :0 ; +if (name) +memcpy (__get_str (name), name->name, name->len); ), TP_printk("dev %d:%d dp ino 0x%llx name %.*s", MAJOR(__entry->dev), MINOR(__entry->dev),
Signature · 6 lines
1.8.3.1 _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs
Re: [PATCH v3 4/4] xfs: fold xfs_create_tmpfile() into xfs_create()
Quoted 11 lines · Brian Foster
xfs_create_tmpfile()duplicates most ofxfs_create()with the exception of a couple minor differences. We use a unique transaction reservation and place the allocated inode on the unlinked list rather than create a directory entry for it.Fold
xfs_create_tmpfile()intoxfs_create()to reduce some of this duplication. The name parameter that represents the name of the directory entry to create is now conditional and its existence dictates whether a directory entry is created for the inode or not.Signed-off-by Brian Foster <[email protected]>
Looks correct although I still can't get excited about it..
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
Quoted 6 lines · Brian Foster
Hi all,
Here's a v3 series for the patches previously posted here:
Quoted 7 lines
Patches 1 and 2 are just a split-up of the v1 patch:
http://oss.sgi.com/archives/xfs/2014-04/msg00149.html
Note the v1 patch has a reviewed-by, so feel free to drop 1 and 2 here in favor of that version. Patches 3 and 4 are a couple cleanups in the
xfs_create()path.
I've already got the orginal patch lined up for commit and have been testing it for a couple of days, so I'll stick with the original.
The other two patches I'll add to the 3.16 queue as they aren't really bug fixes....
Cheers,
Dave.
Signature · 7 lines
Dave Chinner [email protected] _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs