Re: Fixing dht rename of directory symlink

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Your summary is accurate. Currently distribute assumes link(2)ability of non-directories to provide POSIX semantics that the destination path should always be accessible if a file already existed at the time of rename(2). I will review this patch in more detail. In the mean time, a couple of points

1. Can you submit this patch in accordance to the development work flow outlined at http://www.gluster.com/community/documentation/index.php/Development_Work_Flow

2. Can you check if the other file types, like block/char devices, named pipes, named sockets etc are link(2)able? We might have to extend a non-link(2) based solution to the rest of the filetypes.

Avati

On Sun, Jul 31, 2011 at 1:44 PM, Emmanuel Dreyfus <manu@xxxxxxxxxx> wrote:
This is a followup to my previous messages about dht and rename. Here is a
quick summary for the impatient:

dht rename works on non directory files by link/rename/unlink. As I understand
this is to make sure the file is accessible during the rename operation.
However this breaks on the standard front when we rename a symlink to a
directory.

Standards say that link(2) on a directory may not be supported. In is indeed
not supported on Linux ext3fs, nor on NetBSD FFS. Nothing is said in the
standards about link(2) on a symlink to a directory. Linux does it, making two
symlinks with same inode. NetBSD first resolves the symlink, discovers the
link(2) attempt on a directory, and raises EPERM.

Both approaches to link(2) on symlink to a directory are standared compliant,
but glusterfs relying on a given behavior here is a mistake. I talked to
NetBSD crowd about adding a linux_link(2) system call, but that proposal did
not reach consensus. Therefore I would like to fix this in glusterfs.

Here is a first try. It works fine when rename operates on the same subvolume.
It works but never gives control back to the calling process when it happens
of different subvolumes.

Help would be welcome.

--- dht-rename.c.orig   2011-07-31 09:19:02.000000000 +0200
+++ dht-rename.c        2011-07-31 09:46:09.000000000 +0200
@@ -501,9 +501,10 @@

        if (local->call_cnt == 0)
                goto unwind;

-        if (src_cached != dst_hashed && src_cached != dst_cached) {
+        if (!IA_ISLNK(local->loc.inode->ia_type) &&
+           src_cached != dst_hashed && src_cached != dst_cached) {
                gf_log (this->name, GF_LOG_TRACE,
                        "deleting old src datafile %s @ %s",
                        local->loc.path, src_cached->name);

@@ -521,9 +522,9 @@
                            src_hashed, src_hashed->fops->unlink,
                            &local->loc);
        }

-        if (dst_cached
+        if (!IA_ISLNK(local->loc.inode->ia_type) && dst_cached
            && (dst_cached != dst_hashed)
            && (dst_cached != src_cached)) {
                gf_log (this->name, GF_LOG_TRACE,
                        "deleting old dst datafile %s @ %s",
@@ -669,14 +670,25 @@
                                       src_cached, dst_hashed, &local->loc);
        }

        if (src_cached != dst_hashed) {
-                gf_log (this->name, GF_LOG_TRACE,
-                        "link %s => %s (%s)", local->loc.path,
-                        local->loc2.path, src_cached->name);
-                STACK_WIND (frame, dht_rename_links_cbk,
-                            src_cached, src_cached->fops->link,
-                            &local->loc, &local->loc2);
+               if (IA_ISLNK(local->loc.inode->ia_type)) {
+                       gf_log (this->name, GF_LOG_TRACE,
+                               "rename link %s => %s (%s)", local->loc.path,
+                               local->loc2.path, src_cached->name);
+
+                       STACK_WIND (frame, dht_rename_cbk,
+                                   src_cached, src_cached->fops->rename,
+                                   &local->loc, &local->loc2);
+               } else {
+                       gf_log (this->name, GF_LOG_TRACE,
+                               "link %s => %s (%s)", local->loc.path,
+                               local->loc2.path, src_cached->name);
+
+                       STACK_WIND (frame, dht_rename_links_cbk,
+                                   src_cached, src_cached->fops->link,
+                                   &local->loc, &local->loc2);
+               }
        }

 nolinks:
        if (!call_cnt) {
bacasable# q
bacasable# diff -U4 dht-rename.c.orig dht-rename.c
--- dht-rename.c.orig   2011-07-31 09:19:02.000000000 +0200
+++ dht-rename.c        2011-07-31 09:46:09.000000000 +0200
@@ -501,9 +501,10 @@

        if (local->call_cnt == 0)
                goto unwind;

-        if (src_cached != dst_hashed && src_cached != dst_cached) {
+        if (!IA_ISLNK(local->loc.inode->ia_type) &&
+           src_cached != dst_hashed && src_cached != dst_cached) {
                gf_log (this->name, GF_LOG_TRACE,
                        "deleting old src datafile %s @ %s",
                        local->loc.path, src_cached->name);

@@ -521,9 +522,9 @@
                            src_hashed, src_hashed->fops->unlink,
                            &local->loc);
        }

-        if (dst_cached
+        if (!IA_ISLNK(local->loc.inode->ia_type) && dst_cached
            && (dst_cached != dst_hashed)
            && (dst_cached != src_cached)) {
                gf_log (this->name, GF_LOG_TRACE,
                        "deleting old dst datafile %s @ %s",
@@ -669,14 +670,25 @@
                                       src_cached, dst_hashed, &local->loc);
        }

        if (src_cached != dst_hashed) {
-                gf_log (this->name, GF_LOG_TRACE,
-                        "link %s => %s (%s)", local->loc.path,
-                        local->loc2.path, src_cached->name);
-                STACK_WIND (frame, dht_rename_links_cbk,
-                            src_cached, src_cached->fops->link,
-                            &local->loc, &local->loc2);
+               if (IA_ISLNK(local->loc.inode->ia_type)) {
+                       gf_log (this->name, GF_LOG_TRACE,
+                               "rename link %s => %s (%s)", local->loc.path,
+                               local->loc2.path, src_cached->name);
+
+                       STACK_WIND (frame, dht_rename_cbk,
+                                   src_cached, src_cached->fops->rename,
+                                   &local->loc, &local->loc2);
+               } else {
+                       gf_log (this->name, GF_LOG_TRACE,
+                               "link %s => %s (%s)", local->loc.path,
+                               local->loc2.path, src_cached->name);
+
+                       STACK_WIND (frame, dht_rename_links_cbk,
+                                   src_cached, src_cached->fops->link,
+                                   &local->loc, &local->loc2);
+               }
        }

 nolinks:
        if (!call_cnt) {


--
Emmanuel Dreyfus
http://hcpnet.free.fr/pubz
manu@xxxxxxxxxx

_______________________________________________
Gluster-devel mailing list
Gluster-devel@xxxxxxxxxx
https://lists.nongnu.org/mailman/listinfo/gluster-devel


[Index of Archives]     [Gluster Users]     [Ceph Users]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux]     [Linux OMAP]     [Linux MIPS]     [eCos]     [Asterisk Internet PBX]     [Linux API]

  Powered by Linux