[Gluster-devel] Fixing dht rename of directory symlink

Anand Avati anand.avati at gmail.com
Sun Jul 31 12:26:42 UTC 2011


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 at netbsd.org> 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 at netbsd.org
>
> _______________________________________________
> Gluster-devel mailing list
> Gluster-devel at nongnu.org
> https://lists.nongnu.org/mailman/listinfo/gluster-devel
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://supercolony.gluster.org/pipermail/gluster-devel/attachments/20110731/488a4032/attachment-0003.html>


More information about the Gluster-devel mailing list