[PATCH] Shadow Copy Improvements

[James Peach]

On 26/04/2007, at 10:44 AM, Ed Plese wrote:

> Attached is a set of 3 patches that add fixes and improvements to the
> existing shadow_copy VFS module.  I had previously sent a patch for  
> a new
> shadow_copy_zfs module with customizations specific to ZFS on  
> Solaris but
> these patches instead add generic improvements that should work  
> with most
> snapshot methods.
>
> I'd like give a big thanks to Alison Winters for contributing many  
> of the
> ideas for this patch as well as code and testing.

Thankyou both, this is a nice patch. I have some comments below ...

> The defaults for all of the new smb.conf parameters are set to values
> that preserve backwards compatibility with the current module.
>
> The patches are:
>
> 1-dirent-fix.patch
>   This fixes the module to work on systems that define struct  
> dirent with
>   d_name[1].  Solaris is an example of such a system and it causes the
>   share to appear to be completely empty.

This looks OK. I would prefer that you pad out the dirent structures  
to guarantee a safe alignment, 8 or 16 bytes.

> 2-sort.patch
>   With the existing shadow_copy module the shadow copies are  
> displayed in
>   the Windows GUI in the order the server obtains them from readdir 
> (3).
>   On some systems and filesystems readdir(3) does not return files  
> in any
>   particular order which leads to the list in the GUI being  
> unsorted and
>   difficult to use.  This patch allows the list to be sorted based on
>   a "sort" parameter specified for the module.  Allowed values are  
> "asc"
>   or "desc".  When not specified the current unsorted behavior is  
> maintained.

This looks fine too, though I think we should sort by default.

> 3-paths.patch

This needs a copyright attribution.

>   This patch allows for various components of the snapshot paths to be
>   easily customized.  Currently this must be done by creating  
> scripts that
>   will create the appropriate symbolic links every time a snapshot  
> is taken
>   and consequently clean up all of the symbolic links whenever a  
> snapshot
>   is deleted.  With this patch the path components are specified in  
> the
>   smb.conf file using the new parameters "path", "subpath",  
> "format", and
>   "localtime".  The defaults for all of the new parameters maintain  
> the
>   current behavior of the module.
>

In shadow_copy_opendir(), you are still using shadow_copy_match_name  
to filter out shadow directories. It looks like this won't work  
correctly with if you specify the format parameter.

You should be able to cache the prefix created in  
shadow_copy_file_to_snapshot_path since it's quite expensive to  
create. Attaching this to handle->data should be sufficient.

>   path - Specifies the path to the directory that contains the  
> snapshots.
>
>   format - This is the format of the snapshot names in str[fp]time  
> notation
>       except that $ characters are used in place of % characters.

Hmmm. We already use %$FOO to expand environment variables. Is it  
possible for this to get unexpected results?

>   subpath - The subdirectory under the snapshot that contains all  
> of the
>       files for this shadow copy.

Can you elaborate on the need for both a "path" and a "subpath"  
parameter? Why are they both necessary?

>   localtime (boolean) - Treat the snapshot names as being in localtime
>       instead of the default of GMT.
>
>   These probably aren't the clearest explanations of the parameters  
> but the
>   examples below should help to clear up any confusion.
>
> Some limitations include:
>
> * shadow copy does not work correctly when mapping a drive to a
>   subdirectory of a share

Could you add a comment noting this. A future enhancement might be to  
check that the share is the root of a mounted filesystem when the  
module loads.

> * snapshot names are limited to expressions that can be expressed  
> with the
>   str[fp]time(3) functions and variable substitutions in smb.conf
>
> Example uses:
>
> Use with @GMT- directories or symbolic links in the share:
>
> [homes]
>    public = no
>    writable = yes
>    printable = no
>    vfs object = shadow_copy
>
>
> A single large filesystem mounted at /home that contains all of the  
> home
> directories.  The snapshots reside in /snapshots/home.
>
> [homes]
>    path = /home/%U
>    public = no
>    writable = yes
>    printable = no
>    vfs object = shadow_copy
>    shadow_copy: path = /snapshots/home
>    shadow_copy: subpath = %U
>    shadow_copy: format = $Y.$m.$d-$H.$M.$S
>    shadow_copy: sort = desc
>    shadow_copy: localtime = yes
>
>
> A separate ZFS filesystem for each home directory.
>
> [homes]
>    path = /home/%U
>    public = no
>    writable = yes
>    printable = no
>    vfs object = shadow_copy
>    shadow_copy: path = /home/%U/.zfs/snapshot
>    shadow_copy: format = $Y.$m.$d-$H.$M.$S
>    shadow_copy: sort = desc
>    shadow_copy: localtime = yes

As per the limitations noted above, these configs won't work  
correctly, no?

>
>
> Comments and feedback are welcome!
>
> Thanks,
>
> Ed Plese
> <1-dirent-fix.patch>
> <2-sort.patch>
> <3-paths.patch>

--
James Peach | jpeach at samba.org