Bug 1234610 - ACL created on a dht.linkto file on a files that skipped rebalance
ACL created on a dht.linkto file on a files that skipped rebalance
Status: CLOSED ERRATA
Product: Red Hat Gluster Storage
Classification: Red Hat
Component: distribute (Show other bugs)
3.0
Unspecified Unspecified
high Severity high
: ---
: RHGS 3.1.1
Assigned To: Nithya Balachandran
RajeshReddy
: ZStream
Depends On:
Blocks: 1216951 1247563 1251815 1258377
  Show dependency treegraph
 
Reported: 2015-06-22 15:57 EDT by Cedric Buissart
Modified: 2016-07-13 18:34 EDT (History)
11 users (show)

See Also:
Fixed In Version: glusterfs-3.7.1-14
Doc Type: Bug Fix
Doc Text:
Previously, POSIX ACLs set on a file were copied onto the DHT linkto file created when the file was being migrated. This changed the linkto file permissions causing the file to be treated as a data file and file operations to be sent to the wrong file. With this fix, the POSIX ACLs are not set on the DHT linkto files.
Story Points: ---
Clone Of:
: 1247563 (view as bug list)
Environment:
Last Closed: 2015-10-05 03:14:18 EDT
Type: Bug
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---


Attachments (Terms of Use)


External Trackers
Tracker ID Priority Status Summary Last Updated
Red Hat Product Errata RHSA-2015:1845 normal SHIPPED_LIVE Moderate: Red Hat Gluster Storage 3.1 update 2015-10-05 07:06:22 EDT

  None (edit)
Description Cedric Buissart 2015-06-22 15:57:12 EDT
DESCRIPTION

When rebalancing a file with ACL, if you are unlucky enough so that the file is skipped due to __dht_check_free_space() failure (or maybe other reasons), the ACLs will still be migrated on the destination file, containing the dht.linkto XATTR.

This leads to issues where on the client, the file will be seen twice :

---8<----
[root@cbuissar-rhs30-node1 severalfiles]# echo bla > file
[root@cbuissar-rhs30-node1 severalfiles]# setfacl -m u:cedric:w file
[root@cbuissar-rhs30-node1 severalfiles]# ll file
-rw-rw-r--+ 1 root root 4 Jun 22 21:42 file
[root@cbuissar-rhs30-node1 severalfiles]# mv file renamed
[root@cbuissar-rhs30-node1 severalfiles]# gluster volume rebalance thingluster start
volume rebalance: thingluster: success: Rebalance on thingluster has been started successfully. Use rebalance status command to check status of the rebalance process.
ID: 9277c760-2495-4058-894e-12e95e005b90

[root@cbuissar-rhs30-node1 severalfiles]# ls -li | grep renamed
12641300739152549957 -rw-rw-r-T+ 1 root root        4 Jun 22 21:43 renamed
12641300739152549957 -rw-rw-r-T+ 1 root root        4 Jun 22 21:43 renamed
---->8----

looking on the bricks :

on my node 2 :
524419 -rw-rw-r--+ 2 root root 4 Jun 22 21:42 /mnt/bricks/thiny/gluster/severalfiles/renamed

On my node 4 :
524427 -rw-rw-r-T+ 2 root root 4 Jun 22 21:43 /mnt/bricks/thiny/gluster/severalfiles/renamed


We can see that the node 4 inherited from the ACLs, while this usually does not happen : regular dht.linkto files do not have ACLs.
Comment 2 Cedric Buissart 2015-06-22 16:02:04 EDT
I suspect that the issue is a side effect of xlators/cluster/dht/src/dht-rebalance.c , function __dht_rebalance_create_dst_file :

static inline int
__dht_rebalance_create_dst_file (xlator_t *to, xlator_t *from, loc_t *loc, struct iatt *stbuf,
                                 dict_t *dict, fd_t **dst_fd, dict_t *xattr)
{
[...]
        /* TODO: move all xattr related operations to fd based operations */
        ret = syncop_listxattr (from, loc, &xattr);
        if (ret < 0) {
                gf_msg (this->name, GF_LOG_WARNING, 0,
                        DHT_MSG_MIGRATE_FILE_FAILED,
                        "Migrate file failed:"
                        "%s: failed to get xattr from %s (%s)",
                        loc->path, from->name, strerror (-ret));
                ret = -1;
        }

        /* create the destination, with required modes/xattr */
        ret = __dht_rebalance_create_dst_file (to, from, loc, &stbuf,
                                               dict, &dst_fd, xattr);
        if (ret)
                goto out;

        ret = __dht_check_free_space (to, from, loc, &stbuf, flag);
        if (ret) {
                goto out;
        }


Where the xattr are copied over (syncop_listxattr(); __dht_rebalance_create_dst_file()) but then we might cancel the migration if __dht_check_free_space() returns a failure ... leading to xattr having been modified on the destination.

or maybe we want to change the behavior or readdir/getdents ?
Comment 3 Bipin Kunal 2015-06-23 03:47:08 EDT
(In reply to Cedric Buissart from comment #2)
> I suspect that the issue is a side effect of
> xlators/cluster/dht/src/dht-rebalance.c , function
> __dht_rebalance_create_dst_file :
> 
I think the function which Cedric wanted to point is :
int
dht_migrate_file (xlator_t *this, loc_t *loc, xlator_t *from, xlator_t *to,
                  int flag)
> {
> [...]
>         /* TODO: move all xattr related operations to fd based operations */
>         ret = syncop_listxattr (from, loc, &xattr);
>         if (ret < 0) {
>                 gf_msg (this->name, GF_LOG_WARNING, 0,
>                         DHT_MSG_MIGRATE_FILE_FAILED,
>                         "Migrate file failed:"
>                         "%s: failed to get xattr from %s (%s)",
>                         loc->path, from->name, strerror (-ret));
>                 ret = -1;
>         }
> 
>         /* create the destination, with required modes/xattr */
>         ret = __dht_rebalance_create_dst_file (to, from, loc, &stbuf,
>                                                dict, &dst_fd, xattr);
>         if (ret)
>                 goto out;
> 
>         ret = __dht_check_free_space (to, from, loc, &stbuf, flag);
>         if (ret) {
>                 goto out;
>         }
> 
> 
> Where the xattr are copied over (syncop_listxattr();
> __dht_rebalance_create_dst_file()) but then we might cancel the migration if
> __dht_check_free_space() returns a failure ... leading to xattr having been
> modified on the destination.
> 
> or maybe we want to change the behavior or readdir/getdents ?

I think we should be calling function __dht_check_free_space() at least prior to __dht_rebalance_create_dst_file else we should have proper cleanup activity in the "out" section.
 

Thanks,
Bipin Kunal
Comment 5 Cedric Buissart 2015-06-23 10:00:39 EDT
(In reply to Bipin Kunal from comment #3)
> 
> I think we should be calling function __dht_check_free_space() at least
> prior to __dht_rebalance_create_dst_file else we should have proper cleanup
> activity in the "out" section.
>  
> 
> Thanks,
> Bipin Kunal

Yes, I was thinking the same. After a quick try, it seems to work :
I have the "data movement attempted from node [..] with higher disk space to a node" in the logs, but this time, did not lead to dups in the ls output.

However, additionally : should we also look at the readdir behaviour and check why the ACLs on the linkto file leads to showing dups ?
Comment 13 monti lawrence 2015-07-22 16:50:19 EDT
Doc text is edited. Please sign off to be included in Known Issues.
Comment 20 RajeshReddy 2015-09-14 03:52:12 EDT
Tested with build " glusterfs-libs-3.7.1-14" and ran following tests and all tests passed so marking this as a verified 

1. Migration of big file which has posix ACLS and while migration is going on do IO from existing FD and mount it on another client and do FOP 
2. Migration of Big file and while migration is going on do IO from existing FD and make sure destination brick is down (after transfer of first half modify content in the first half)
3. Migration of Big file and while migration is going do IO from new client and while IO is going on make sure destination brick is down (Modify both already transferred data and new data)
4. While migration of big file is going on change the ACLs (modify and delete few)
5. while migration of big file which has ACL change the ACL (modify and delete few)
6. Abort migration of big file and check the clean up 
7. Abort the migration of big file which has ACLs
8. Make sure that by renaming the file it has both link and actual file and then change the ACL of the re-named file
Comment 21 Divya 2015-09-23 03:09:21 EDT
Please review and sign-off the edited doc text.
Comment 22 Nithya Balachandran 2015-09-28 05:00:30 EDT
Made some minor edits.
Comment 24 errata-xmlrpc 2015-10-05 03:14:18 EDT
Since the problem described in this bug report should be
resolved in a recent advisory, it has been closed with a
resolution of ERRATA.

For information on the advisory, and where to find the updated
files, follow the link below.

If the solution does not work for you, open a new bug report.

https://rhn.redhat.com/errata/RHSA-2015-1845.html

Note You need to log in before you can comment on or make changes to this bug.