Discussion:
[PATCH 0/2] Add support to RENAME_EXCHANGE flat to XFS
Carlos Maiolino
2014-10-15 18:17:20 UTC
Permalink
This patchset aims to implement RENAME_EXCHANGE support (from sys_renameat2) to
XFS.

For this to be achieved, XFS need to export a rename2() method, which I included
in the first patch.

The second patch is the real implementation of the RENAME_EXCHANGE flags, which
most of the work I based on xfs_rename().

I'd like some suggestions about the way I handled journaling, probably
XFS_TRANS_RENAME is not the best way to log the changes during the
RENAME_EXCHANGE, and we might need a new journal type for this specific reason.

BTW, this patchset passed the xfstests 23, 24 and 25 (specifically for
RENAME_EXCHANGE), and I also tested the projectID inheritance problem, where
both paths must be under the same projectID to be able to change (I'm going to
implement this test into the xfstests too).

Carlos Maiolino (2):
xfs_vn_rename by xfs_vn_rename2
Add support to RENAME_EXCHANGE flag

fs/xfs/xfs_inode.c | 187 +++++++++++++++++++++++++++++++++++++++++++++++++++++
fs/xfs/xfs_inode.h | 4 ++
fs/xfs/xfs_iops.c | 22 +++++--
3 files changed, 207 insertions(+), 6 deletions(-)
--
2.1.0
Carlos Maiolino
2014-10-15 18:17:21 UTC
Permalink
To be able to support RENAME_EXCHANGE flag from renameat2() system call, XFS
must have its inode_operations updated, exporting .rename2 method, instead of
.rename.

This patch just replaces the (now old) .rename method by .rename2, using the
same infra-structure, but checking rename flags.

calls to .rename2 using RENAME_EXCHANGE flag, although now handled inside XFS,
still returns -EINVAL.

RENAME_NOREPLACE is handled via VFS and we don't need to care about it inside
xfs_vn_rename2.

Signed-off-by: Carlos Maiolino <***@redhat.com>
---
fs/xfs/xfs_iops.c | 17 +++++++++++------
1 file changed, 11 insertions(+), 6 deletions(-)

diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
index 7212949..b2b92c7 100644
--- a/fs/xfs/xfs_iops.c
+++ b/fs/xfs/xfs_iops.c
@@ -379,22 +379,27 @@ xfs_vn_symlink(
}

STATIC int
-xfs_vn_rename(
+xfs_vn_rename2(
struct inode *odir,
struct dentry *odentry,
struct inode *ndir,
- struct dentry *ndentry)
+ struct dentry *ndentry,
+ unsigned int flags)
{
struct inode *new_inode = ndentry->d_inode;
struct xfs_name oname;
struct xfs_name nname;

+ /* XFS does not support RENAME_EXCHANGE yet */
+ if (flags & ~RENAME_NOREPLACE)
+ return -EINVAL;
+
xfs_dentry_to_name(&oname, odentry, 0);
xfs_dentry_to_name(&nname, ndentry, odentry->d_inode->i_mode);

return xfs_rename(XFS_I(odir), &oname, XFS_I(odentry->d_inode),
- XFS_I(ndir), &nname, new_inode ?
- XFS_I(new_inode) : NULL);
+ XFS_I(ndir), &nname,
+ new_inode ? XFS_I(new_inode) : NULL);
}

/*
@@ -1117,7 +1122,7 @@ static const struct inode_operations xfs_dir_inode_operations = {
*/
.rmdir = xfs_vn_unlink,
.mknod = xfs_vn_mknod,
- .rename = xfs_vn_rename,
+ .rename2 = xfs_vn_rename2,
.get_acl = xfs_get_acl,
.set_acl = xfs_set_acl,
.getattr = xfs_vn_getattr,
@@ -1145,7 +1150,7 @@ static const struct inode_operations xfs_dir_ci_inode_operations = {
*/
.rmdir = xfs_vn_unlink,
.mknod = xfs_vn_mknod,
- .rename = xfs_vn_rename,
+ .rename2 = xfs_vn_rename2,
.get_acl = xfs_get_acl,
.set_acl = xfs_set_acl,
.getattr = xfs_vn_getattr,
--
2.1.0
Brian Foster
2014-10-16 21:04:57 UTC
Permalink
Post by Carlos Maiolino
To be able to support RENAME_EXCHANGE flag from renameat2() system call, XFS
must have its inode_operations updated, exporting .rename2 method, instead of
.rename.
This patch just replaces the (now old) .rename method by .rename2, using the
same infra-structure, but checking rename flags.
calls to .rename2 using RENAME_EXCHANGE flag, although now handled inside XFS,
still returns -EINVAL.
RENAME_NOREPLACE is handled via VFS and we don't need to care about it inside
xfs_vn_rename2.
---
fs/xfs/xfs_iops.c | 17 +++++++++++------
1 file changed, 11 insertions(+), 6 deletions(-)
diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
index 7212949..b2b92c7 100644
--- a/fs/xfs/xfs_iops.c
+++ b/fs/xfs/xfs_iops.c
@@ -379,22 +379,27 @@ xfs_vn_symlink(
}
STATIC int
-xfs_vn_rename(
+xfs_vn_rename2(
The function rename seems unnecessary..? Meh, not a big deal to me
either way. Otherwise, this one seems Ok.

Brian
Post by Carlos Maiolino
struct inode *odir,
struct dentry *odentry,
struct inode *ndir,
- struct dentry *ndentry)
+ struct dentry *ndentry,
+ unsigned int flags)
{
struct inode *new_inode = ndentry->d_inode;
struct xfs_name oname;
struct xfs_name nname;
+ /* XFS does not support RENAME_EXCHANGE yet */
+ if (flags & ~RENAME_NOREPLACE)
+ return -EINVAL;
+
xfs_dentry_to_name(&oname, odentry, 0);
xfs_dentry_to_name(&nname, ndentry, odentry->d_inode->i_mode);
return xfs_rename(XFS_I(odir), &oname, XFS_I(odentry->d_inode),
- XFS_I(ndir), &nname, new_inode ?
- XFS_I(new_inode) : NULL);
+ XFS_I(ndir), &nname,
+ new_inode ? XFS_I(new_inode) : NULL);
}
/*
@@ -1117,7 +1122,7 @@ static const struct inode_operations xfs_dir_inode_operations = {
*/
.rmdir = xfs_vn_unlink,
.mknod = xfs_vn_mknod,
- .rename = xfs_vn_rename,
+ .rename2 = xfs_vn_rename2,
.get_acl = xfs_get_acl,
.set_acl = xfs_set_acl,
.getattr = xfs_vn_getattr,
@@ -1145,7 +1150,7 @@ static const struct inode_operations xfs_dir_ci_inode_operations = {
*/
.rmdir = xfs_vn_unlink,
.mknod = xfs_vn_mknod,
- .rename = xfs_vn_rename,
+ .rename2 = xfs_vn_rename2,
.get_acl = xfs_get_acl,
.set_acl = xfs_set_acl,
.getattr = xfs_vn_getattr,
--
2.1.0
_______________________________________________
xfs mailing list
http://oss.sgi.com/mailman/listinfo/xfs
Christoph Hellwig
2014-10-17 09:35:56 UTC
Permalink
Post by Brian Foster
The function rename seems unnecessary..? Meh, not a big deal to me
either way. Otherwise, this one seems Ok.
Yeah, we might as well keep the old name. As far as I'm concerned
I'd love to get rid of the two different IOPS in the VFS, too.
Carlos Maiolino
2014-10-21 12:56:54 UTC
Permalink
Hi Brian, Chris.

Sorry my delay to reply, I was in a software conference this week, and barely
accessed my e-mails.

I can certainly re-do this patch to avoid the name change.

I used a new name to follow the VFS convention, although I also agree we
'should' get rig of several versions and keep just newer updates to the same
syscalls.

So, I'll re-do this patch and send a V2.

Cheers
Post by Christoph Hellwig
Post by Brian Foster
The function rename seems unnecessary..? Meh, not a big deal to me
either way. Otherwise, this one seems Ok.
Yeah, we might as well keep the old name. As far as I'm concerned
I'd love to get rid of the two different IOPS in the VFS, too.
_______________________________________________
xfs mailing list
http://oss.sgi.com/mailman/listinfo/xfs
--
Carlos
Carlos Maiolino
2014-10-15 18:17:22 UTC
Permalink
Adds a new function named xfs_cross_rename(), responsible to handle requests
from sys_renameat2() using RENAME_EXCHANGE flag.

Signed-off-by: Carlos Maiolino <***@redhat.com>
---
fs/xfs/xfs_inode.c | 187 +++++++++++++++++++++++++++++++++++++++++++++++++++++
fs/xfs/xfs_inode.h | 4 ++
fs/xfs/xfs_iops.c | 7 +-
3 files changed, 197 insertions(+), 1 deletion(-)

diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
index fea3c92..a5bc88d 100644
--- a/fs/xfs/xfs_inode.c
+++ b/fs/xfs/xfs_inode.c
@@ -2920,6 +2920,193 @@ xfs_rename(
return error;
}

+/* xfs_cross_rename()
+ *
+ * responsible to handle RENAME_EXCHANGE flag
+ * in renameat2() sytemcall
+ */
+int
+xfs_cross_rename(
+ xfs_inode_t *src_dp,
+ struct xfs_name *src_name,
+ xfs_inode_t *src_ip,
+ xfs_inode_t *target_dp,
+ struct xfs_name *target_name,
+ xfs_inode_t *target_ip)
+{
+ xfs_trans_t *tp = NULL;
+ xfs_mount_t *mp = src_dp->i_mount;
+ int new_parent; /* Crossing from different parents */
+ int src_is_directory;
+ int tgt_is_directory;
+ int error;
+ xfs_bmap_free_t free_list;
+ xfs_fsblock_t first_block;
+ int cancel_flags;
+ int committed;
+ xfs_inode_t *inodes[4];
+ int spaceres;
+ int num_inodes;
+
+ new_parent = (src_dp != target_dp);
+ src_is_directory = S_ISDIR(src_ip->i_d.di_mode);
+ tgt_is_directory = S_ISDIR(target_ip->i_d.di_mode);
+
+ xfs_sort_for_rename(src_dp, target_dp, src_ip, target_ip,
+ inodes, &num_inodes);
+
+ xfs_bmap_init(&free_list, &first_block);
+ tp = xfs_trans_alloc(mp, XFS_TRANS_RENAME);
+ cancel_flags = XFS_TRANS_RELEASE_LOG_RES;
+ spaceres = XFS_RENAME_SPACE_RES(mp, target_name->len);
+ error = xfs_trans_reserve(tp, &M_RES(mp)->tr_rename, spaceres, 0);
+
+ if (error == -ENOSPC) {
+ spaceres = 0;
+ error = xfs_trans_reserve(tp, &M_RES(mp)->tr_rename, 0, 0);
+ }
+ if (error) {
+ xfs_trans_cancel(tp, 0);
+ goto std_return;
+ }
+
+ /*
+ * Attach the dquots to the inodes
+ */
+ error = xfs_qm_vop_rename_dqattach(inodes);
+ if (error) {
+ xfs_trans_cancel(tp, cancel_flags);
+ goto std_return;
+ }
+
+ /*
+ * Lock all participating inodes. In case of RENAME_EXCHANGE, target
+ * must exist, so we'll be locking at least 3 inodes here.
+ */
+ xfs_lock_inodes(inodes, num_inodes, XFS_ILOCK_EXCL);
+
+ /*
+ * Join all the inodes to the transaction. From this point on,
+ * we can rely on either trans_commit or trans_cancel to unlock
+ * them.
+ * target_ip will always exist, so, no need to check its existence.
+ */
+ xfs_trans_ijoin(tp, src_dp, XFS_ILOCK_EXCL);
+ if (new_parent)
+ xfs_trans_ijoin(tp, target_dp, XFS_ILOCK_EXCL);
+
+ xfs_trans_ijoin(tp, src_ip, XFS_ILOCK_EXCL);
+ xfs_trans_ijoin(tp, target_ip, XFS_ILOCK_EXCL);
+
+ /*
+ * If we are using project inheritance, we only allow RENAME_EXCHANGE
+ * into our tree when the project IDs are the same; else the tree quota
+ * mechanism would be circumvented.
+ */
+ if (unlikely(((target_dp->i_d.di_flags & XFS_DIFLAG_PROJINHERIT) ||
+ (src_dp->i_d.di_flags & XFS_DIFLAG_PROJINHERIT)) &&
+ (xfs_get_projid(src_dp) != xfs_get_projid(target_dp)) )) {
+ error = -EXDEV;
+ goto error_return;
+ }
+
+ error = xfs_dir_replace(tp, src_dp, src_name,
+ target_ip->i_ino,
+ &first_block, &free_list, spaceres);
+ if (error)
+ goto abort_return;
+
+ /*
+ * Update ".." entry to match the new parent
+ */
+ if (new_parent && tgt_is_directory) {
+ error = xfs_dir_replace(tp, target_ip, &xfs_name_dotdot,
+ src_dp->i_ino, &first_block, &free_list, spaceres);
+ if (error)
+ goto abort_return;
+ }
+
+ xfs_trans_ichgtime(tp, src_dp, XFS_ICHGTIME_MOD | XFS_ICHGTIME_CHG);
+
+ error = xfs_dir_replace(tp, target_dp, target_name,
+ src_ip->i_ino,
+ &first_block, &free_list, spaceres);
+ if (error)
+ goto abort_return;
+
+ /*
+ * Update ".." entry to match the new parent
+ */
+ if (new_parent && src_is_directory) {
+ error = xfs_dir_replace(tp, src_ip, &xfs_name_dotdot,
+ target_dp->i_ino, &first_block, &free_list, spaceres);
+ if (error)
+ goto abort_return;
+ }
+
+ /*
+ * In case we are crossing different file types between different
+ * parents, we must update parent's link count to match the ".."
+ * entry of the new child (or the removal of it).
+ */
+ if (new_parent) {
+ xfs_trans_ichgtime(tp, target_dp, XFS_ICHGTIME_MOD | XFS_ICHGTIME_CHG);
+
+ if (src_is_directory && !tgt_is_directory) {
+ error = xfs_droplink(tp, src_dp);
+ if (error)
+ goto abort_return;
+ error = xfs_bumplink(tp, target_dp);
+ if (error)
+ goto abort_return;
+ }
+
+ if (tgt_is_directory && !src_is_directory) {
+ error = xfs_droplink(tp, target_dp);
+ if (error)
+ goto abort_return;
+ error = xfs_bumplink(tp, src_dp);
+ if (error)
+ goto abort_return;
+ }
+
+ /*
+ * We don't need to log the source dir if
+ * this is the same as the target.
+ */
+ xfs_trans_log_inode(tp, src_dp, XFS_ILOG_CORE);
+ }
+
+ xfs_trans_log_inode(tp, target_dp, XFS_ILOG_CORE);
+ xfs_trans_log_inode(tp, src_ip, XFS_ILOG_CORE);
+ xfs_trans_log_inode(tp, target_ip, XFS_ILOG_CORE);
+
+ /*
+ * If this is a synchronous mount, make sure the rename transaction goes
+ * to disk before returning to the user.
+ */
+ if (mp->m_flags & (XFS_MOUNT_WSYNC|XFS_MOUNT_DIRSYNC))
+ xfs_trans_set_sync(tp);
+
+ error = xfs_bmap_finish(&tp, &free_list, &committed);
+ if (error) {
+ xfs_bmap_cancel(&free_list);
+ xfs_trans_cancel(tp, (XFS_TRANS_RELEASE_LOG_RES | XFS_TRANS_ABORT));
+ goto std_return;
+ }
+
+ return xfs_trans_commit(tp, XFS_TRANS_RELEASE_LOG_RES);
+
+abort_return:
+ cancel_flags |= XFS_TRANS_ABORT;
+error_return:
+ xfs_bmap_cancel(&free_list);
+ xfs_trans_cancel(tp, cancel_flags);
+std_return:
+ return error;
+
+}
+
STATIC int
xfs_iflush_cluster(
xfs_inode_t *ip,
diff --git a/fs/xfs/xfs_inode.h b/fs/xfs/xfs_inode.h
index c10e3fa..16889d3 100644
--- a/fs/xfs/xfs_inode.h
+++ b/fs/xfs/xfs_inode.h
@@ -341,6 +341,10 @@ int xfs_rename(struct xfs_inode *src_dp, struct xfs_name *src_name,
struct xfs_inode *src_ip, struct xfs_inode *target_dp,
struct xfs_name *target_name,
struct xfs_inode *target_ip);
+int xfs_cross_rename(struct xfs_inode *src_dp, struct xfs_name *src_name,
+ struct xfs_inode *src_ip, struct xfs_inode *target_dp,
+ struct xfs_name *target_name,
+ struct xfs_inode *target_ip);

void xfs_ilock(xfs_inode_t *, uint);
int xfs_ilock_nowait(xfs_inode_t *, uint);
diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
index b2b92c7..bc164df 100644
--- a/fs/xfs/xfs_iops.c
+++ b/fs/xfs/xfs_iops.c
@@ -391,12 +391,17 @@ xfs_vn_rename2(
struct xfs_name nname;

/* XFS does not support RENAME_EXCHANGE yet */
- if (flags & ~RENAME_NOREPLACE)
+ if (flags & ~(RENAME_NOREPLACE | RENAME_EXCHANGE))
return -EINVAL;

xfs_dentry_to_name(&oname, odentry, 0);
xfs_dentry_to_name(&nname, ndentry, odentry->d_inode->i_mode);

+ if (flags & RENAME_EXCHANGE)
+ return xfs_cross_rename(XFS_I(odir), &oname, XFS_I(odentry->d_inode),
+ XFS_I(ndir), &nname,
+ new_inode ? XFS_I(new_inode) : NULL);
+
return xfs_rename(XFS_I(odir), &oname, XFS_I(odentry->d_inode),
XFS_I(ndir), &nname,
new_inode ? XFS_I(new_inode) : NULL);
--
2.1.0
Brian Foster
2014-10-16 21:05:37 UTC
Permalink
Post by Carlos Maiolino
Adds a new function named xfs_cross_rename(), responsible to handle requests
from sys_renameat2() using RENAME_EXCHANGE flag.
---
Hi Carlos,

Some high-level comments from a first pass...
Post by Carlos Maiolino
fs/xfs/xfs_inode.c | 187 +++++++++++++++++++++++++++++++++++++++++++++++++++++
fs/xfs/xfs_inode.h | 4 ++
fs/xfs/xfs_iops.c | 7 +-
3 files changed, 197 insertions(+), 1 deletion(-)
diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
index fea3c92..a5bc88d 100644
--- a/fs/xfs/xfs_inode.c
+++ b/fs/xfs/xfs_inode.c
@@ -2920,6 +2920,193 @@ xfs_rename(
return error;
}
+/* xfs_cross_rename()
+ *
+ * responsible to handle RENAME_EXCHANGE flag
+ * in renameat2() sytemcall
+ */
+int
+xfs_cross_rename(
+ xfs_inode_t *src_dp,
+ struct xfs_name *src_name,
+ xfs_inode_t *src_ip,
+ xfs_inode_t *target_dp,
+ struct xfs_name *target_name,
+ xfs_inode_t *target_ip)
+{
+ xfs_trans_t *tp = NULL;
+ xfs_mount_t *mp = src_dp->i_mount;
+ int new_parent; /* Crossing from different parents */
+ int src_is_directory;
+ int tgt_is_directory;
+ int error;
+ xfs_bmap_free_t free_list;
+ xfs_fsblock_t first_block;
+ int cancel_flags;
+ int committed;
+ xfs_inode_t *inodes[4];
+ int spaceres;
+ int num_inodes;
+
+ new_parent = (src_dp != target_dp);
+ src_is_directory = S_ISDIR(src_ip->i_d.di_mode);
+ tgt_is_directory = S_ISDIR(target_ip->i_d.di_mode);
+
+ xfs_sort_for_rename(src_dp, target_dp, src_ip, target_ip,
+ inodes, &num_inodes);
+
+ xfs_bmap_init(&free_list, &first_block);
+ tp = xfs_trans_alloc(mp, XFS_TRANS_RENAME);
+ cancel_flags = XFS_TRANS_RELEASE_LOG_RES;
+ spaceres = XFS_RENAME_SPACE_RES(mp, target_name->len);
+ error = xfs_trans_reserve(tp, &M_RES(mp)->tr_rename, spaceres, 0);
+
It seems to me that the existing block and log reservations would
"cover" the rename exchange case, but it might be worth defining new
reservations for the purpose of clarity and to prevent future problems.

XFS_RENAME_SPACE_RES() covers directory removal and insertion. Here we
are doing neither, which makes me wonder whether we need a block
reservation at all. It does appear that we have a sf dir case where the
inode number could cause a format conversion. Perhaps we need something
that calculates the blocks required for the insertion of the max of both
names (it seems like the conversion would only happen once, but we don't
know which way)? I haven't spent a ton of time in directory code, so I
could easily be missing something.

The tr_rename log reservation considers four inodes, two directory
modifications, a target inode unlink (the overwrite case), and alloc
btree mods for directory blocks being freed. IIUC, the exchange case
should only ever log four inodes and the possible dir format conversion
(e.g., no unlink, no dir block frees). We could define a new
tr_rename_xchg reservation that encodes that and documents it
appropriately in the comment.

It might be worth getting a second opinion from Dave or somebody before
going too far ahead on the logging work...
Post by Carlos Maiolino
+ if (error == -ENOSPC) {
+ spaceres = 0;
+ error = xfs_trans_reserve(tp, &M_RES(mp)->tr_rename, 0, 0);
+ }
+ if (error) {
+ xfs_trans_cancel(tp, 0);
+ goto std_return;
+ }
+
+ /*
+ * Attach the dquots to the inodes
+ */
+ error = xfs_qm_vop_rename_dqattach(inodes);
+ if (error) {
+ xfs_trans_cancel(tp, cancel_flags);
+ goto std_return;
+ }
+
+ /*
+ * Lock all participating inodes. In case of RENAME_EXCHANGE, target
+ * must exist, so we'll be locking at least 3 inodes here.
+ */
+ xfs_lock_inodes(inodes, num_inodes, XFS_ILOCK_EXCL);
+
+ /*
+ * Join all the inodes to the transaction. From this point on,
+ * we can rely on either trans_commit or trans_cancel to unlock
+ * them.
+ * target_ip will always exist, so, no need to check its existence.
+ */
+ xfs_trans_ijoin(tp, src_dp, XFS_ILOCK_EXCL);
+ if (new_parent)
+ xfs_trans_ijoin(tp, target_dp, XFS_ILOCK_EXCL);
+
+ xfs_trans_ijoin(tp, src_ip, XFS_ILOCK_EXCL);
+ xfs_trans_ijoin(tp, target_ip, XFS_ILOCK_EXCL);
+
+ /*
+ * If we are using project inheritance, we only allow RENAME_EXCHANGE
+ * into our tree when the project IDs are the same; else the tree quota
+ * mechanism would be circumvented.
+ */
+ if (unlikely(((target_dp->i_d.di_flags & XFS_DIFLAG_PROJINHERIT) ||
+ (src_dp->i_d.di_flags & XFS_DIFLAG_PROJINHERIT)) &&
+ (xfs_get_projid(src_dp) != xfs_get_projid(target_dp)) )) {
+ error = -EXDEV;
+ goto error_return;
+ }
+
I think that having a separate helper for the rename exchange case is
generally the right thing. That said, I wonder if we're splitting things
at the right level because it looks like xfs_rename() could handle
everything we have in xfs_cross_rename() up to about this point.

I definitely don't think we should go too far and try to handle all of
this in one function, even if there is some duplication in the directory
name replacement and inode link management. The logic would probably end
up unnecessarily hairy and difficult to reason about.
Post by Carlos Maiolino
+ error = xfs_dir_replace(tp, src_dp, src_name,
+ target_ip->i_ino,
+ &first_block, &free_list, spaceres);
+ if (error)
+ goto abort_return;
+
+ /*
+ * Update ".." entry to match the new parent
+ */
+ if (new_parent && tgt_is_directory) {
+ error = xfs_dir_replace(tp, target_ip, &xfs_name_dotdot,
+ src_dp->i_ino, &first_block, &free_list, spaceres);
+ if (error)
+ goto abort_return;
+ }
+
+ xfs_trans_ichgtime(tp, src_dp, XFS_ICHGTIME_MOD | XFS_ICHGTIME_CHG);
+
+ error = xfs_dir_replace(tp, target_dp, target_name,
+ src_ip->i_ino,
+ &first_block, &free_list, spaceres);
+ if (error)
+ goto abort_return;
+
+ /*
+ * Update ".." entry to match the new parent
+ */
+ if (new_parent && src_is_directory) {
+ error = xfs_dir_replace(tp, src_ip, &xfs_name_dotdot,
+ target_dp->i_ino, &first_block, &free_list, spaceres);
+ if (error)
+ goto abort_return;
+ }
+
+ /*
+ * In case we are crossing different file types between different
+ * parents, we must update parent's link count to match the ".."
+ * entry of the new child (or the removal of it).
+ */
+ if (new_parent) {
+ xfs_trans_ichgtime(tp, target_dp, XFS_ICHGTIME_MOD | XFS_ICHGTIME_CHG);
+
+ if (src_is_directory && !tgt_is_directory) {
+ error = xfs_droplink(tp, src_dp);
+ if (error)
+ goto abort_return;
+ error = xfs_bumplink(tp, target_dp);
+ if (error)
+ goto abort_return;
+ }
+
+ if (tgt_is_directory && !src_is_directory) {
+ error = xfs_droplink(tp, target_dp);
+ if (error)
+ goto abort_return;
+ error = xfs_bumplink(tp, src_dp);
+ if (error)
+ goto abort_return;
+ }
+
+ /*
+ * We don't need to log the source dir if
+ * this is the same as the target.
+ */
+ xfs_trans_log_inode(tp, src_dp, XFS_ILOG_CORE);
+ }
+
+ xfs_trans_log_inode(tp, target_dp, XFS_ILOG_CORE);
+ xfs_trans_log_inode(tp, src_ip, XFS_ILOG_CORE);
+ xfs_trans_log_inode(tp, target_ip, XFS_ILOG_CORE);
+
... and from here to the end also looks equivalent to xfs_rename().
Could we do something like pass the flags (or some new parameter) to
xfs_rename() and convert the meat of the directory update calls into a
couple internal helpers? For example:

xfs_rename(...)
{
/* setup tp, lock inodes, etc. */

if (rename_exchange)
error = xfs_rename_exchange_int(...);
else
error = xfs_rename(...);
if (error)
...

/* tp completion handling */

return xfs_trans_commit(...);

abort_return:
...

return error;
}

... and that could be done with another refactoring patch to prepare
xfs_rename(). Just a thought.
Post by Carlos Maiolino
+ /*
+ * If this is a synchronous mount, make sure the rename transaction goes
+ * to disk before returning to the user.
+ */
+ if (mp->m_flags & (XFS_MOUNT_WSYNC|XFS_MOUNT_DIRSYNC))
+ xfs_trans_set_sync(tp);
+
+ error = xfs_bmap_finish(&tp, &free_list, &committed);
+ if (error) {
+ xfs_bmap_cancel(&free_list);
+ xfs_trans_cancel(tp, (XFS_TRANS_RELEASE_LOG_RES | XFS_TRANS_ABORT));
+ goto std_return;
+ }
+
+ return xfs_trans_commit(tp, XFS_TRANS_RELEASE_LOG_RES);
+
+ cancel_flags |= XFS_TRANS_ABORT;
+ xfs_bmap_cancel(&free_list);
+ xfs_trans_cancel(tp, cancel_flags);
+ return error;
+
+}
+
STATIC int
xfs_iflush_cluster(
xfs_inode_t *ip,
diff --git a/fs/xfs/xfs_inode.h b/fs/xfs/xfs_inode.h
index c10e3fa..16889d3 100644
--- a/fs/xfs/xfs_inode.h
+++ b/fs/xfs/xfs_inode.h
@@ -341,6 +341,10 @@ int xfs_rename(struct xfs_inode *src_dp, struct xfs_name *src_name,
struct xfs_inode *src_ip, struct xfs_inode *target_dp,
struct xfs_name *target_name,
struct xfs_inode *target_ip);
+int xfs_cross_rename(struct xfs_inode *src_dp, struct xfs_name *src_name,
+ struct xfs_inode *src_ip, struct xfs_inode *target_dp,
+ struct xfs_name *target_name,
+ struct xfs_inode *target_ip);
void xfs_ilock(xfs_inode_t *, uint);
int xfs_ilock_nowait(xfs_inode_t *, uint);
diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
index b2b92c7..bc164df 100644
--- a/fs/xfs/xfs_iops.c
+++ b/fs/xfs/xfs_iops.c
@@ -391,12 +391,17 @@ xfs_vn_rename2(
struct xfs_name nname;
/* XFS does not support RENAME_EXCHANGE yet */
- if (flags & ~RENAME_NOREPLACE)
+ if (flags & ~(RENAME_NOREPLACE | RENAME_EXCHANGE))
return -EINVAL;
xfs_dentry_to_name(&oname, odentry, 0);
This might need to be handled differently for the exchange case. As
below, the new dentry always gets the old mode. I suspect we don't care
about the mode of the original dentry in traditional rename since that
entry goes away. It looks like it would be set to 0 here in the exchange
case, however, rather than ndentry->d_inode->i_mode (which we can't
assume exists for the non-exchange case).

Brian
Post by Carlos Maiolino
xfs_dentry_to_name(&nname, ndentry, odentry->d_inode->i_mode);
+ if (flags & RENAME_EXCHANGE)
+ return xfs_cross_rename(XFS_I(odir), &oname, XFS_I(odentry->d_inode),
+ XFS_I(ndir), &nname,
+ new_inode ? XFS_I(new_inode) : NULL);
+
return xfs_rename(XFS_I(odir), &oname, XFS_I(odentry->d_inode),
XFS_I(ndir), &nname,
new_inode ? XFS_I(new_inode) : NULL);
--
2.1.0
_______________________________________________
xfs mailing list
http://oss.sgi.com/mailman/listinfo/xfs
Dave Chinner
2014-10-20 00:25:28 UTC
Permalink
Post by Brian Foster
Post by Carlos Maiolino
Adds a new function named xfs_cross_rename(), responsible to handle requests
from sys_renameat2() using RENAME_EXCHANGE flag.
---
Hi Carlos,
Some high-level comments from a first pass...
Post by Carlos Maiolino
fs/xfs/xfs_inode.c | 187 +++++++++++++++++++++++++++++++++++++++++++++++++++++
fs/xfs/xfs_inode.h | 4 ++
fs/xfs/xfs_iops.c | 7 +-
3 files changed, 197 insertions(+), 1 deletion(-)
diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
index fea3c92..a5bc88d 100644
--- a/fs/xfs/xfs_inode.c
+++ b/fs/xfs/xfs_inode.c
@@ -2920,6 +2920,193 @@ xfs_rename(
return error;
}
+/* xfs_cross_rename()
+ *
+ * responsible to handle RENAME_EXCHANGE flag
+ * in renameat2() sytemcall
+ */
+int
+xfs_cross_rename(
+ xfs_inode_t *src_dp,
+ struct xfs_name *src_name,
+ xfs_inode_t *src_ip,
+ xfs_inode_t *target_dp,
+ struct xfs_name *target_name,
+ xfs_inode_t *target_ip)
+{
+ xfs_trans_t *tp = NULL;
+ xfs_mount_t *mp = src_dp->i_mount;
+ int new_parent; /* Crossing from different parents */
+ int src_is_directory;
+ int tgt_is_directory;
+ int error;
+ xfs_bmap_free_t free_list;
+ xfs_fsblock_t first_block;
+ int cancel_flags;
+ int committed;
+ xfs_inode_t *inodes[4];
+ int spaceres;
+ int num_inodes;
+
+ new_parent = (src_dp != target_dp);
+ src_is_directory = S_ISDIR(src_ip->i_d.di_mode);
+ tgt_is_directory = S_ISDIR(target_ip->i_d.di_mode);
+
+ xfs_sort_for_rename(src_dp, target_dp, src_ip, target_ip,
+ inodes, &num_inodes);
+
+ xfs_bmap_init(&free_list, &first_block);
+ tp = xfs_trans_alloc(mp, XFS_TRANS_RENAME);
+ cancel_flags = XFS_TRANS_RELEASE_LOG_RES;
+ spaceres = XFS_RENAME_SPACE_RES(mp, target_name->len);
+ error = xfs_trans_reserve(tp, &M_RES(mp)->tr_rename, spaceres, 0);
+
It seems to me that the existing block and log reservations would
"cover" the rename exchange case, but it might be worth defining new
reservations for the purpose of clarity and to prevent future problems.
XFS_RENAME_SPACE_RES() covers directory removal and insertion. Here we
are doing neither, which makes me wonder whether we need a block
reservation at all. It does appear that we have a sf dir case where the
inode number could cause a format conversion. Perhaps we need something
that calculates the blocks required for the insertion of the max of both
names (it seems like the conversion would only happen once, but we don't
know which way)? I haven't spent a ton of time in directory code, so I
could easily be missing something.
The shortform replace can result in shortform->block conversion,
therefore we need the reservation.
Post by Brian Foster
The tr_rename log reservation considers four inodes, two directory
modifications, a target inode unlink (the overwrite case), and alloc
btree mods for directory blocks being freed. IIUC, the exchange case
should only ever log four inodes and the possible dir format conversion
(e.g., no unlink, no dir block frees). We could define a new
tr_rename_xchg reservation that encodes that and documents it
appropriately in the comment.
The rename log reservation is the worse case that a rename operation
requires - it is not specific to a particular rename instance. This
new reanme type fits within the existing definition, so we should
just use it unchanged.

What it comes down to is that there is no point in trying to define
reservations for every single possible type of operation we can do -
it's just too much maintenance overhead to verify that they are
correct after some incidental change. If we define the worst case,
then everything else is covered and we don't have to care about
whether we have the reservation for a specific case right, or indeed
whether we are using the correct reservation for a specific rename
transaction....
Post by Brian Foster
Post by Carlos Maiolino
+ if (error == -ENOSPC) {
+ spaceres = 0;
+ error = xfs_trans_reserve(tp, &M_RES(mp)->tr_rename, 0, 0);
+ }
+ if (error) {
+ xfs_trans_cancel(tp, 0);
+ goto std_return;
+ }
This is not necessary. The spaceres == 0 case in the rename is for
adding new directory entries at ENOSPC and that is checked by
xfs_dir_canenter(). We are not calling that function (because we
aren't adding a name) and therefore we can't run without a full
space reservation.

Oh, and kill that "std_return" name.

if (error) {
cancel_flags = 0;
goto out_trans_cancel;
}
Post by Brian Foster
Post by Carlos Maiolino
+
+ /*
+ * Attach the dquots to the inodes
+ */
+ error = xfs_qm_vop_rename_dqattach(inodes);
+ if (error) {
+ xfs_trans_cancel(tp, cancel_flags);
+ goto std_return;
+ }
if (error)
goto out_trans_cancel;
Post by Brian Foster
Post by Carlos Maiolino
+
+ /*
+ * Lock all participating inodes. In case of RENAME_EXCHANGE, target
+ * must exist, so we'll be locking at least 3 inodes here.
+ */
+ xfs_lock_inodes(inodes, num_inodes, XFS_ILOCK_EXCL);
+
+ /*
+ * Join all the inodes to the transaction. From this point on,
+ * we can rely on either trans_commit or trans_cancel to unlock
+ * them.
+ * target_ip will always exist, so, no need to check its existence.
+ */
+ xfs_trans_ijoin(tp, src_dp, XFS_ILOCK_EXCL);
+ if (new_parent)
+ xfs_trans_ijoin(tp, target_dp, XFS_ILOCK_EXCL);
+
+ xfs_trans_ijoin(tp, src_ip, XFS_ILOCK_EXCL);
+ xfs_trans_ijoin(tp, target_ip, XFS_ILOCK_EXCL);
+
+ /*
+ * If we are using project inheritance, we only allow RENAME_EXCHANGE
+ * into our tree when the project IDs are the same; else the tree quota
+ * mechanism would be circumvented.
+ */
+ if (unlikely(((target_dp->i_d.di_flags & XFS_DIFLAG_PROJINHERIT) ||
+ (src_dp->i_d.di_flags & XFS_DIFLAG_PROJINHERIT)) &&
+ (xfs_get_projid(src_dp) != xfs_get_projid(target_dp)) )) {
+ error = -EXDEV;
+ goto error_return;
+ }
+
I think that having a separate helper for the rename exchange case is
generally the right thing. That said, I wonder if we're splitting things
at the right level because it looks like xfs_rename() could handle
everything we have in xfs_cross_rename() up to about this point.
Right. I think that splitting out the internal part of xfs_rename
after all this common setup code is the best way to proceed.
Post by Brian Foster
I definitely don't think we should go too far and try to handle all of
this in one function, even if there is some duplication in the directory
name replacement and inode link management. The logic would probably end
up unnecessarily hairy and difficult to reason about.
Post by Carlos Maiolino
+ error = xfs_dir_replace(tp, src_dp, src_name,
+ target_ip->i_ino,
+ &first_block, &free_list, spaceres);
+ if (error)
+ goto abort_return;
+
+ /*
+ * Update ".." entry to match the new parent
+ */
+ if (new_parent && tgt_is_directory) {
+ error = xfs_dir_replace(tp, target_ip, &xfs_name_dotdot,
+ src_dp->i_ino, &first_block, &free_list, spaceres);
+ if (error)
+ goto abort_return;
+ }
+
+ xfs_trans_ichgtime(tp, src_dp, XFS_ICHGTIME_MOD | XFS_ICHGTIME_CHG);
+
+ error = xfs_dir_replace(tp, target_dp, target_name,
+ src_ip->i_ino,
+ &first_block, &free_list, spaceres);
+ if (error)
+ goto abort_return;
+
+ /*
+ * Update ".." entry to match the new parent
+ */
+ if (new_parent && src_is_directory) {
+ error = xfs_dir_replace(tp, src_ip, &xfs_name_dotdot,
+ target_dp->i_ino, &first_block, &free_list, spaceres);
+ if (error)
+ goto abort_return;
+ }
So you do a bunch of work based on new_parent and
Post by Brian Foster
Post by Carlos Maiolino
+
+ /*
+ * In case we are crossing different file types between different
+ * parents, we must update parent's link count to match the ".."
+ * entry of the new child (or the removal of it).
+ */
+ if (new_parent) {
+ xfs_trans_ichgtime(tp, target_dp, XFS_ICHGTIME_MOD | XFS_ICHGTIME_CHG);
+
+ if (src_is_directory && !tgt_is_directory) {
+ error = xfs_droplink(tp, src_dp);
+ if (error)
+ goto abort_return;
[whitespace is screwed up]
Post by Brian Foster
Post by Carlos Maiolino
+ error = xfs_bumplink(tp, target_dp);
+ if (error)
+ goto abort_return;
+ }
+
+ if (tgt_is_directory && !src_is_directory) {
+ error = xfs_droplink(tp, target_dp);
+ if (error)
+ goto abort_return;
+ error = xfs_bumplink(tp, src_dp);
+ if (error)
+ goto abort_return;
+ }
+
+ /*
+ * We don't need to log the source dir if
+ * this is the same as the target.
+ */
+ xfs_trans_log_inode(tp, src_dp, XFS_ILOG_CORE);
+ }
You do a bunch more work based on the same variables. THis should
reall ybe combined into a single set of logic to manipulate the
directory states.

if (new_parent) {
if (tgt_is_directory) {
error = xfs_dir_replace(tp, target_ip, &xfs_name_dotdot,
src_dp->i_ino, &first_block, &free_list, spaceres);
if (error)
goto out_abort;
if (!src_is_directory) {
error = xfs_droplink(tp, target_dp);
if (error)
goto out_abort;
error = xfs_bumplink(tp, src_dp);
if (error)
goto out_abort;
}
}

if (src_is_directory) {
error = xfs_dir_replace(tp, src_ip, &xfs_name_dotdot,
target_dp->i_ino, &first_block, &free_list, spaceres);
if (error)
goto out_abort;
.....
Post by Brian Foster
Post by Carlos Maiolino
+ /*
+ * If this is a synchronous mount, make sure the rename transaction goes
+ * to disk before returning to the user.
+ */
+ if (mp->m_flags & (XFS_MOUNT_WSYNC|XFS_MOUNT_DIRSYNC))
+ xfs_trans_set_sync(tp);
+
+ error = xfs_bmap_finish(&tp, &free_list, &committed);
+ if (error) {
+ xfs_bmap_cancel(&free_list);
+ xfs_trans_cancel(tp, (XFS_TRANS_RELEASE_LOG_RES | XFS_TRANS_ABORT));
+ goto std_return;
+ }
+ return xfs_trans_commit(tp, XFS_TRANS_RELEASE_LOG_RES);
error = xfs_bmap_finish(&tp, &free_list, &committed);
if (!error)
return xfs_trans_commit(tp, XFS_TRANS_RELEASE_LOG_RES);
Post by Brian Foster
Post by Carlos Maiolino
+ cancel_flags |= XFS_TRANS_ABORT;
+ xfs_bmap_cancel(&free_list);
+ xfs_trans_cancel(tp, cancel_flags);
+ return error;
out_abort:
cancel_flags |= XFS_TRANS_ABORT;
out_bmap_cancel:
xfs_bmap_cancel(&free_list);
out_trans_cancel:
xfs_trans_cancel(tp, cancel_flags);
return error;

Cheers,

Dave.
--
Dave Chinner
***@fromorbit.com
Carlos Maiolino
2014-10-21 13:02:15 UTC
Permalink
Thanks for the review guys, I'm going to apply the suggestions and send a V2
Post by Dave Chinner
Post by Brian Foster
Post by Carlos Maiolino
Adds a new function named xfs_cross_rename(), responsible to handle requests
from sys_renameat2() using RENAME_EXCHANGE flag.
---
Hi Carlos,
Some high-level comments from a first pass...
Post by Carlos Maiolino
fs/xfs/xfs_inode.c | 187 +++++++++++++++++++++++++++++++++++++++++++++++++++++
fs/xfs/xfs_inode.h | 4 ++
fs/xfs/xfs_iops.c | 7 +-
3 files changed, 197 insertions(+), 1 deletion(-)
diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
index fea3c92..a5bc88d 100644
--- a/fs/xfs/xfs_inode.c
+++ b/fs/xfs/xfs_inode.c
@@ -2920,6 +2920,193 @@ xfs_rename(
return error;
}
+/* xfs_cross_rename()
+ *
+ * responsible to handle RENAME_EXCHANGE flag
+ * in renameat2() sytemcall
+ */
+int
+xfs_cross_rename(
+ xfs_inode_t *src_dp,
+ struct xfs_name *src_name,
+ xfs_inode_t *src_ip,
+ xfs_inode_t *target_dp,
+ struct xfs_name *target_name,
+ xfs_inode_t *target_ip)
+{
+ xfs_trans_t *tp = NULL;
+ xfs_mount_t *mp = src_dp->i_mount;
+ int new_parent; /* Crossing from different parents */
+ int src_is_directory;
+ int tgt_is_directory;
+ int error;
+ xfs_bmap_free_t free_list;
+ xfs_fsblock_t first_block;
+ int cancel_flags;
+ int committed;
+ xfs_inode_t *inodes[4];
+ int spaceres;
+ int num_inodes;
+
+ new_parent = (src_dp != target_dp);
+ src_is_directory = S_ISDIR(src_ip->i_d.di_mode);
+ tgt_is_directory = S_ISDIR(target_ip->i_d.di_mode);
+
+ xfs_sort_for_rename(src_dp, target_dp, src_ip, target_ip,
+ inodes, &num_inodes);
+
+ xfs_bmap_init(&free_list, &first_block);
+ tp = xfs_trans_alloc(mp, XFS_TRANS_RENAME);
+ cancel_flags = XFS_TRANS_RELEASE_LOG_RES;
+ spaceres = XFS_RENAME_SPACE_RES(mp, target_name->len);
+ error = xfs_trans_reserve(tp, &M_RES(mp)->tr_rename, spaceres, 0);
+
It seems to me that the existing block and log reservations would
"cover" the rename exchange case, but it might be worth defining new
reservations for the purpose of clarity and to prevent future problems.
XFS_RENAME_SPACE_RES() covers directory removal and insertion. Here we
are doing neither, which makes me wonder whether we need a block
reservation at all. It does appear that we have a sf dir case where the
inode number could cause a format conversion. Perhaps we need something
that calculates the blocks required for the insertion of the max of both
names (it seems like the conversion would only happen once, but we don't
know which way)? I haven't spent a ton of time in directory code, so I
could easily be missing something.
The shortform replace can result in shortform->block conversion,
therefore we need the reservation.
Post by Brian Foster
The tr_rename log reservation considers four inodes, two directory
modifications, a target inode unlink (the overwrite case), and alloc
btree mods for directory blocks being freed. IIUC, the exchange case
should only ever log four inodes and the possible dir format conversion
(e.g., no unlink, no dir block frees). We could define a new
tr_rename_xchg reservation that encodes that and documents it
appropriately in the comment.
The rename log reservation is the worse case that a rename operation
requires - it is not specific to a particular rename instance. This
new reanme type fits within the existing definition, so we should
just use it unchanged.
What it comes down to is that there is no point in trying to define
reservations for every single possible type of operation we can do -
it's just too much maintenance overhead to verify that they are
correct after some incidental change. If we define the worst case,
then everything else is covered and we don't have to care about
whether we have the reservation for a specific case right, or indeed
whether we are using the correct reservation for a specific rename
transaction....
Post by Brian Foster
Post by Carlos Maiolino
+ if (error == -ENOSPC) {
+ spaceres = 0;
+ error = xfs_trans_reserve(tp, &M_RES(mp)->tr_rename, 0, 0);
+ }
+ if (error) {
+ xfs_trans_cancel(tp, 0);
+ goto std_return;
+ }
This is not necessary. The spaceres == 0 case in the rename is for
adding new directory entries at ENOSPC and that is checked by
xfs_dir_canenter(). We are not calling that function (because we
aren't adding a name) and therefore we can't run without a full
space reservation.
Oh, and kill that "std_return" name.
if (error) {
cancel_flags = 0;
goto out_trans_cancel;
}
Post by Brian Foster
Post by Carlos Maiolino
+
+ /*
+ * Attach the dquots to the inodes
+ */
+ error = xfs_qm_vop_rename_dqattach(inodes);
+ if (error) {
+ xfs_trans_cancel(tp, cancel_flags);
+ goto std_return;
+ }
if (error)
goto out_trans_cancel;
Post by Brian Foster
Post by Carlos Maiolino
+
+ /*
+ * Lock all participating inodes. In case of RENAME_EXCHANGE, target
+ * must exist, so we'll be locking at least 3 inodes here.
+ */
+ xfs_lock_inodes(inodes, num_inodes, XFS_ILOCK_EXCL);
+
+ /*
+ * Join all the inodes to the transaction. From this point on,
+ * we can rely on either trans_commit or trans_cancel to unlock
+ * them.
+ * target_ip will always exist, so, no need to check its existence.
+ */
+ xfs_trans_ijoin(tp, src_dp, XFS_ILOCK_EXCL);
+ if (new_parent)
+ xfs_trans_ijoin(tp, target_dp, XFS_ILOCK_EXCL);
+
+ xfs_trans_ijoin(tp, src_ip, XFS_ILOCK_EXCL);
+ xfs_trans_ijoin(tp, target_ip, XFS_ILOCK_EXCL);
+
+ /*
+ * If we are using project inheritance, we only allow RENAME_EXCHANGE
+ * into our tree when the project IDs are the same; else the tree quota
+ * mechanism would be circumvented.
+ */
+ if (unlikely(((target_dp->i_d.di_flags & XFS_DIFLAG_PROJINHERIT) ||
+ (src_dp->i_d.di_flags & XFS_DIFLAG_PROJINHERIT)) &&
+ (xfs_get_projid(src_dp) != xfs_get_projid(target_dp)) )) {
+ error = -EXDEV;
+ goto error_return;
+ }
+
I think that having a separate helper for the rename exchange case is
generally the right thing. That said, I wonder if we're splitting things
at the right level because it looks like xfs_rename() could handle
everything we have in xfs_cross_rename() up to about this point.
Right. I think that splitting out the internal part of xfs_rename
after all this common setup code is the best way to proceed.
Post by Brian Foster
I definitely don't think we should go too far and try to handle all of
this in one function, even if there is some duplication in the directory
name replacement and inode link management. The logic would probably end
up unnecessarily hairy and difficult to reason about.
Post by Carlos Maiolino
+ error = xfs_dir_replace(tp, src_dp, src_name,
+ target_ip->i_ino,
+ &first_block, &free_list, spaceres);
+ if (error)
+ goto abort_return;
+
+ /*
+ * Update ".." entry to match the new parent
+ */
+ if (new_parent && tgt_is_directory) {
+ error = xfs_dir_replace(tp, target_ip, &xfs_name_dotdot,
+ src_dp->i_ino, &first_block, &free_list, spaceres);
+ if (error)
+ goto abort_return;
+ }
+
+ xfs_trans_ichgtime(tp, src_dp, XFS_ICHGTIME_MOD | XFS_ICHGTIME_CHG);
+
+ error = xfs_dir_replace(tp, target_dp, target_name,
+ src_ip->i_ino,
+ &first_block, &free_list, spaceres);
+ if (error)
+ goto abort_return;
+
+ /*
+ * Update ".." entry to match the new parent
+ */
+ if (new_parent && src_is_directory) {
+ error = xfs_dir_replace(tp, src_ip, &xfs_name_dotdot,
+ target_dp->i_ino, &first_block, &free_list, spaceres);
+ if (error)
+ goto abort_return;
+ }
So you do a bunch of work based on new_parent and
Post by Brian Foster
Post by Carlos Maiolino
+
+ /*
+ * In case we are crossing different file types between different
+ * parents, we must update parent's link count to match the ".."
+ * entry of the new child (or the removal of it).
+ */
+ if (new_parent) {
+ xfs_trans_ichgtime(tp, target_dp, XFS_ICHGTIME_MOD | XFS_ICHGTIME_CHG);
+
+ if (src_is_directory && !tgt_is_directory) {
+ error = xfs_droplink(tp, src_dp);
+ if (error)
+ goto abort_return;
[whitespace is screwed up]
Post by Brian Foster
Post by Carlos Maiolino
+ error = xfs_bumplink(tp, target_dp);
+ if (error)
+ goto abort_return;
+ }
+
+ if (tgt_is_directory && !src_is_directory) {
+ error = xfs_droplink(tp, target_dp);
+ if (error)
+ goto abort_return;
+ error = xfs_bumplink(tp, src_dp);
+ if (error)
+ goto abort_return;
+ }
+
+ /*
+ * We don't need to log the source dir if
+ * this is the same as the target.
+ */
+ xfs_trans_log_inode(tp, src_dp, XFS_ILOG_CORE);
+ }
You do a bunch more work based on the same variables. THis should
reall ybe combined into a single set of logic to manipulate the
directory states.
if (new_parent) {
if (tgt_is_directory) {
error = xfs_dir_replace(tp, target_ip, &xfs_name_dotdot,
src_dp->i_ino, &first_block, &free_list, spaceres);
if (error)
goto out_abort;
if (!src_is_directory) {
error = xfs_droplink(tp, target_dp);
if (error)
goto out_abort;
error = xfs_bumplink(tp, src_dp);
if (error)
goto out_abort;
}
}
if (src_is_directory) {
error = xfs_dir_replace(tp, src_ip, &xfs_name_dotdot,
target_dp->i_ino, &first_block, &free_list, spaceres);
if (error)
goto out_abort;
.....
Post by Brian Foster
Post by Carlos Maiolino
+ /*
+ * If this is a synchronous mount, make sure the rename transaction goes
+ * to disk before returning to the user.
+ */
+ if (mp->m_flags & (XFS_MOUNT_WSYNC|XFS_MOUNT_DIRSYNC))
+ xfs_trans_set_sync(tp);
+
+ error = xfs_bmap_finish(&tp, &free_list, &committed);
+ if (error) {
+ xfs_bmap_cancel(&free_list);
+ xfs_trans_cancel(tp, (XFS_TRANS_RELEASE_LOG_RES | XFS_TRANS_ABORT));
+ goto std_return;
+ }
+ return xfs_trans_commit(tp, XFS_TRANS_RELEASE_LOG_RES);
error = xfs_bmap_finish(&tp, &free_list, &committed);
if (!error)
return xfs_trans_commit(tp, XFS_TRANS_RELEASE_LOG_RES);
Post by Brian Foster
Post by Carlos Maiolino
+ cancel_flags |= XFS_TRANS_ABORT;
+ xfs_bmap_cancel(&free_list);
+ xfs_trans_cancel(tp, cancel_flags);
+ return error;
cancel_flags |= XFS_TRANS_ABORT;
xfs_bmap_cancel(&free_list);
xfs_trans_cancel(tp, cancel_flags);
return error;
Cheers,
Dave.
--
Dave Chinner
_______________________________________________
xfs mailing list
http://oss.sgi.com/mailman/listinfo/xfs
--
Carlos
Loading...