Anders,
Please find the modified patch to be applied on master for the SGID bit propagation issue, https://bugzilla.redhat.com/show_bug.cgi?id=1110262
Other comments inline.
> > DHT winds a call to mkdir as a part of the dht_selfheal_directory (in
> > dht_selfheal_dir_mkdir where it winds a call to mkdir for all
> > subvolumes that have the directory missing) with the right mode bits
> > (in this case with the SGID bit). As the POSIX layer on the brick
> > calls mkdir, the SGID bit is not set for the newly created directory
> > due to [1].
> I think this depends on the sgid bit of the parent directory on the brick,
> which might indicate that mkdir_p should be checked as well.
Yes, if the parent directory has the SGID bit this would not happen, but in the case explained, when we heal the parent that first starts carrying the SGID bit, we start the problem and hence children do not get the cascaded SGID set.
>
> > Further to calling mkdir DHT now winds an setattr to set the mode
> > bits straight, but ends up using the mode bits that are returned in
> > the iatt (stat) information by the just concluded mkdir wind, which
> > has the SGID bit missing, as mkdir returns the stat information from
> > posix_mkdir, by doing a stat post mkdir. Hence we never end up
> > setting the SGID bit in the setattr part of DHT.
> To me this does not quite explain how a directory (sometimes) winds up with
> permissions set to 0.
Agreed, hence the initial comment on answering one of the multiple problems posed :)
>
> > This would make the
> > directory equal on all the bricks, and further discrepancies from the
> > mount point or on the backed should not be seen.
> Make sure to use the last version (currently 3) of the test script from
> https://bugzilla.redhat.com/show_bug.cgi?id=1110262
Did the same and things work as intended, but for some reason the waitperm never exited even with the right permissions, but I validated that part manually and things looked good.
Regards,
Shyam
diff --git a/xlators/cluster/dht/src/dht-common.c b/xlators/cluster/dht/src/dht-common.c
index 7eb40fa..be48782 100644
--- a/xlators/cluster/dht/src/dht-common.c
+++ b/xlators/cluster/dht/src/dht-common.c
@@ -577,6 +577,28 @@ out:
}
int
+permission_changed (ia_prot_t *local, ia_prot_t *stbuf)
+{
+ if((local->owner.read != stbuf->owner.read) ||
+ (local->owner.write != stbuf->owner.write) ||
+ (local->owner.exec != stbuf->owner.exec) ||
+ (local->group.read != stbuf->group.read) ||
+ (local->group.write != stbuf->group.write) ||
+ (local->group.exec != stbuf->group.exec) ||
+ (local->other.read != stbuf->other.read) ||
+ (local->other.write != stbuf->other.write) ||
+ (local->other.exec != stbuf->other.exec ) ||
+ (local->suid != stbuf->suid) ||
+ (local->sgid != stbuf->sgid) ||
+ (local->sticky != stbuf->sticky))
+ {
+ return 1;
+ } else {
+ return 0;
+ }
+}
+
+int
dht_revalidate_cbk (call_frame_t *frame, void *cookie, xlator_t *this,
int op_ret, int op_errno,
inode_t *inode, struct iatt *stbuf, dict_t *xattr,
@@ -677,12 +699,15 @@ dht_revalidate_cbk (call_frame_t *frame, void *cookie, xlator_t *this,
stbuf->ia_ctime_nsec)) {
local->prebuf.ia_gid = stbuf->ia_gid;
local->prebuf.ia_uid = stbuf->ia_uid;
+ local->prebuf.ia_prot = stbuf->ia_prot;
}
}
if (local->stbuf.ia_type != IA_INVAL)
{
if ((local->stbuf.ia_gid != stbuf->ia_gid) ||
- (local->stbuf.ia_uid != stbuf->ia_uid)) {
+ (local->stbuf.ia_uid != stbuf->ia_uid) ||
+ (permission_changed (&(local->stbuf.ia_prot)
+ , (&(stbuf->ia_prot))))) {
local->need_selfheal = 1;
}
}
@@ -730,6 +755,7 @@ out:
uuid_copy (local->gfid, local->stbuf.ia_gfid);
local->stbuf.ia_gid = local->prebuf.ia_gid;
local->stbuf.ia_uid = local->prebuf.ia_uid;
+ local->stbuf.ia_prot = local->prebuf.ia_prot;
copy = create_frame (this, this->ctx->pool);
if (copy) {
copy_local = dht_local_init (copy, &local->loc,
diff --git a/xlators/cluster/dht/src/dht-selfheal.c b/xlators/cluster/dht/src/dht-selfheal.c
index 1e2d743..41c26b3 100644
--- a/xlators/cluster/dht/src/dht-selfheal.c
+++ b/xlators/cluster/dht/src/dht-selfheal.c
@@ -575,7 +575,7 @@ dht_selfheal_dir_mkdir_cbk (call_frame_t *frame, void *cookie, xlator_t *this,
goto out;
}
- dht_iatt_merge (this, &local->stbuf, stbuf, prev->this);
+ //dht_iatt_merge (this, &local->stbuf, stbuf, prev->this);
dht_iatt_merge (this, &local->preparent, preparent, prev->this);
dht_iatt_merge (this, &local->postparent, postparent, prev->this);
@@ -1276,7 +1276,7 @@ dht_dir_attr_heal (void *data)
if (!subvol || (subvol == dht_first_up_subvol (this)))
continue;
ret = syncop_setattr (subvol, &local->loc, &local->stbuf,
- (GF_SET_ATTR_UID | GF_SET_ATTR_GID),
+ (GF_SET_ATTR_UID | GF_SET_ATTR_GID | GF_SET_ATTR_MODE),
NULL, NULL);
if (ret) {
uuid_unparse(local->loc.gfid, gfid);
_______________________________________________
Gluster-devel mailing list
Gluster-devel@xxxxxxxxxxx
http://supercolony.gluster.org/mailman/listinfo/gluster-devel
