From e76a777f3820e62948256a45a38d5e97f3eb08a9 Mon Sep 17 00:00:00 2001 From: N Balachandran Date: Tue, 28 Aug 2018 12:00:33 +0530 Subject: [PATCH 438/444] cluster/dht: In rename, unlink after creating linkto file The linkto file creation for the dst was done in parallel with the unlink of the old src linkto. If these operations reached the brick out of order, we end up with a dst linkto file without a .glusterfs handle. Fixed by unlinking only after the linkto file creation has completed. upstream patch: https://review.gluster.org/#/c/glusterfs/+/21023/ > Change-Id: I4246f7655f5bc180f5ded7fd34d263b7828a8110 > fixes: bz#1621981 > Signed-off-by: N Balachandran Change-Id: Ia845a68bb314997cadab57887a84dff9373400c4 BUG: 1622001 Signed-off-by: N Balachandran Reviewed-on: https://code.engineering.redhat.com/gerrit/154933 Tested-by: RHGS Build Bot Reviewed-by: Raghavendra Gowdappa --- tests/bugs/posix/bug-1619720.t | 1 + xlators/cluster/dht/src/dht-rename.c | 254 ++++++++++++++++++----------------- 2 files changed, 133 insertions(+), 122 deletions(-) diff --git a/tests/bugs/posix/bug-1619720.t b/tests/bugs/posix/bug-1619720.t index 5e0d0f7..bfd304d 100755 --- a/tests/bugs/posix/bug-1619720.t +++ b/tests/bugs/posix/bug-1619720.t @@ -48,6 +48,7 @@ TEST mv $M0/tmp/file-2 $M0/tmp/file-3 TEST mv -f $M0/tmp/file-1 $M0/tmp/file-3 + TEST getfattr -n $pgfid_xattr_name $B0/${V0}0/tmp/file-3 TEST getfattr -n $pgfid_xattr_name $B0/${V0}1/tmp/file-3 diff --git a/xlators/cluster/dht/src/dht-rename.c b/xlators/cluster/dht/src/dht-rename.c index 1d0c2bb..378cb0a 100644 --- a/xlators/cluster/dht/src/dht-rename.c +++ b/xlators/cluster/dht/src/dht-rename.c @@ -849,8 +849,8 @@ dht_rename_cleanup (call_frame_t *frame) if (src_cached == dst_cached) goto nolinks; - if (local->linked && (dst_hashed != src_hashed )&& - (dst_hashed != src_cached)) { + if (local->linked && (dst_hashed != src_hashed) && + (dst_hashed != src_cached)) { call_cnt++; } @@ -935,6 +935,120 @@ nolinks: int +dht_rename_unlink (call_frame_t *frame, xlator_t *this) +{ + dht_local_t *local = NULL; + xlator_t *src_hashed = NULL; + xlator_t *src_cached = NULL; + xlator_t *dst_hashed = NULL; + xlator_t *dst_cached = NULL; + xlator_t *rename_subvol = NULL; + dict_t *xattr = NULL; + + local = frame->local; + + src_hashed = local->src_hashed; + src_cached = local->src_cached; + dst_hashed = local->dst_hashed; + dst_cached = local->dst_cached; + + local->call_cnt = 0; + + /* NOTE: rename_subvol is the same subvolume from which dht_rename_cbk + * is called. since rename has already happened on rename_subvol, + * unlink shouldn't be sent for oldpath (either linkfile or cached-file) + * on rename_subvol. */ + if (src_cached == dst_cached) + rename_subvol = src_cached; + else + rename_subvol = dst_hashed; + + /* TODO: delete files in background */ + + if (src_cached != dst_hashed && src_cached != dst_cached) + local->call_cnt++; + + if (src_hashed != rename_subvol && src_hashed != src_cached) + local->call_cnt++; + + if (dst_cached && dst_cached != dst_hashed && dst_cached != src_cached) + local->call_cnt++; + + if (local->call_cnt == 0) + goto unwind; + + DHT_MARK_FOP_INTERNAL (xattr); + + if (src_cached != dst_hashed && src_cached != dst_cached) { + dict_t *xattr_new = NULL; + + xattr_new = dict_copy_with_ref (xattr, NULL); + + gf_msg_trace (this->name, 0, + "deleting old src datafile %s @ %s", + local->loc.path, src_cached->name); + + if (gf_uuid_compare (local->loc.pargfid, + local->loc2.pargfid) == 0) { + DHT_MARKER_DONT_ACCOUNT(xattr_new); + } + + DHT_CHANGELOG_TRACK_AS_RENAME(xattr_new, &local->loc, + &local->loc2); + STACK_WIND_COOKIE (frame, dht_rename_unlink_cbk, src_cached, + src_cached, src_cached->fops->unlink, + &local->loc, 0, xattr_new); + + dict_unref (xattr_new); + xattr_new = NULL; + } + + if (src_hashed != rename_subvol && src_hashed != src_cached) { + dict_t *xattr_new = NULL; + + xattr_new = dict_copy_with_ref (xattr, NULL); + + gf_msg_trace (this->name, 0, + "deleting old src linkfile %s @ %s", + local->loc.path, src_hashed->name); + + DHT_MARKER_DONT_ACCOUNT(xattr_new); + + STACK_WIND_COOKIE (frame, dht_rename_unlink_cbk, src_hashed, + src_hashed, src_hashed->fops->unlink, + &local->loc, 0, xattr_new); + + dict_unref (xattr_new); + xattr_new = NULL; + } + + if (dst_cached && + (dst_cached != dst_hashed) && + (dst_cached != src_cached)) { + gf_msg_trace (this->name, 0, + "deleting old dst datafile %s @ %s", + local->loc2.path, dst_cached->name); + + STACK_WIND_COOKIE (frame, dht_rename_unlink_cbk, dst_cached, + dst_cached, dst_cached->fops->unlink, + &local->loc2, 0, xattr); + } + if (xattr) + dict_unref (xattr); + return 0; + +unwind: + WIPE (&local->preoldparent); + WIPE (&local->postoldparent); + WIPE (&local->preparent); + WIPE (&local->postparent); + + dht_rename_done (frame, this); + + return 0; +} + +int dht_rename_links_create_cbk (call_frame_t *frame, void *cookie, xlator_t *this, int32_t op_ret, int32_t op_errno, inode_t *inode, struct iatt *stbuf, @@ -947,6 +1061,7 @@ dht_rename_links_create_cbk (call_frame_t *frame, void *cookie, xlator_t *this, prev = cookie; local = frame->local; + /* TODO: Handle this case in lookup-optimize */ if (op_ret == -1) { gf_msg (this->name, GF_LOG_WARNING, op_errno, DHT_MSG_CREATE_LINK_FAILED, @@ -958,8 +1073,8 @@ dht_rename_links_create_cbk (call_frame_t *frame, void *cookie, xlator_t *this, local->linked = _gf_false; dht_linkfile_attr_heal (frame, this); } - DHT_STACK_DESTROY (frame); + dht_rename_unlink (frame, this); return 0; } @@ -973,19 +1088,14 @@ dht_rename_cbk (call_frame_t *frame, void *cookie, xlator_t *this, { dht_local_t *local = NULL; xlator_t *prev = NULL; - xlator_t *src_hashed = NULL; xlator_t *src_cached = NULL; xlator_t *dst_hashed = NULL; xlator_t *dst_cached = NULL; - xlator_t *rename_subvol = NULL; - call_frame_t *link_frame = NULL; - dht_local_t *link_local = NULL; - dict_t *xattr = NULL; + loc_t link_loc = {0}; local = frame->local; prev = cookie; - src_hashed = local->src_hashed; src_cached = local->src_cached; dst_hashed = local->dst_hashed; dst_cached = local->dst_cached; @@ -1043,31 +1153,6 @@ dht_rename_cbk (call_frame_t *frame, void *cookie, xlator_t *this, local->xattr = dict_copy_with_ref (xdata, local->xattr); } - if ((src_cached == dst_cached) && (dst_hashed != dst_cached)) { - link_frame = copy_frame (frame); - if (!link_frame) { - goto err; - } - - /* fop value sent as maxvalue because it is not used - anywhere in this case */ - link_local = dht_local_init (link_frame, &local->loc2, NULL, - GF_FOP_MAXVALUE); - if (!link_local) { - goto err; - } - - if (link_local->loc.inode) - inode_unref (link_local->loc.inode); - link_local->loc.inode = inode_ref (local->loc.inode); - gf_uuid_copy (link_local->gfid, local->loc.inode->gfid); - - dht_linkfile_create (link_frame, dht_rename_links_create_cbk, - this, src_cached, dst_hashed, - &link_local->loc); - } - -err: /* Merge attrs only from src_cached. In case there of src_cached != * dst_hashed, this ignores linkfile attrs. */ if (prev == src_cached) { @@ -1080,98 +1165,23 @@ err: dht_iatt_merge (this, &local->postparent, postnewparent, prev); } + /* Create the linkto file for the dst file */ + if ((src_cached == dst_cached) && (dst_hashed != dst_cached)) { - /* NOTE: rename_subvol is the same subvolume from which dht_rename_cbk - * is called. since rename has already happened on rename_subvol, - * unlink should not be sent for oldpath (either linkfile or cached-file) - * on rename_subvol. */ - if (src_cached == dst_cached) - rename_subvol = src_cached; - else - rename_subvol = dst_hashed; - - /* TODO: delete files in background */ - - if (src_cached != dst_hashed && src_cached != dst_cached) - local->call_cnt++; - - if (src_hashed != rename_subvol && src_hashed != src_cached) - local->call_cnt++; - - if (dst_cached && dst_cached != dst_hashed && dst_cached != src_cached) - local->call_cnt++; - - if (local->call_cnt == 0) - goto unwind; - - DHT_MARK_FOP_INTERNAL (xattr); - - if (src_cached != dst_hashed && src_cached != dst_cached) { - dict_t *xattr_new = NULL; - - xattr_new = dict_copy_with_ref (xattr, NULL); - - gf_msg_trace (this->name, 0, - "deleting old src datafile %s @ %s", - local->loc.path, src_cached->name); - - if (gf_uuid_compare (local->loc.pargfid, - local->loc2.pargfid) == 0) { - DHT_MARKER_DONT_ACCOUNT(xattr_new); - } - - DHT_CHANGELOG_TRACK_AS_RENAME(xattr_new, &local->loc, - &local->loc2); - STACK_WIND_COOKIE (frame, dht_rename_unlink_cbk, src_cached, - src_cached, src_cached->fops->unlink, - &local->loc, 0, xattr_new); - - dict_unref (xattr_new); - xattr_new = NULL; - } - - if (src_hashed != rename_subvol && src_hashed != src_cached) { - dict_t *xattr_new = NULL; - - xattr_new = dict_copy_with_ref (xattr, NULL); - - gf_msg_trace (this->name, 0, - "deleting old src linkfile %s @ %s", - local->loc.path, src_hashed->name); - - DHT_MARKER_DONT_ACCOUNT(xattr_new); - - STACK_WIND_COOKIE (frame, dht_rename_unlink_cbk, src_hashed, - src_hashed, src_hashed->fops->unlink, - &local->loc, 0, xattr_new); - - dict_unref (xattr_new); - xattr_new = NULL; - } + loc_copy (&link_loc, &local->loc2); + if (link_loc.inode) + inode_unref (link_loc.inode); + link_loc.inode = inode_ref (local->loc.inode); + gf_uuid_copy (local->gfid, local->loc.inode->gfid); + gf_uuid_copy (link_loc.gfid, local->loc.inode->gfid); - if (dst_cached - && (dst_cached != dst_hashed) - && (dst_cached != src_cached)) { - gf_msg_trace (this->name, 0, - "deleting old dst datafile %s @ %s", - local->loc2.path, dst_cached->name); - - STACK_WIND_COOKIE (frame, dht_rename_unlink_cbk, dst_cached, - dst_cached, dst_cached->fops->unlink, - &local->loc2, 0, xattr); + dht_linkfile_create (frame, dht_rename_links_create_cbk, + this, src_cached, dst_hashed, + &link_loc); + return 0; } - if (xattr) - dict_unref (xattr); - return 0; - -unwind: - WIPE (&local->preoldparent); - WIPE (&local->postoldparent); - WIPE (&local->preparent); - WIPE (&local->postparent); - - dht_rename_done (frame, this); + dht_rename_unlink (frame, this); return 0; cleanup: -- 1.8.3.1