Patch to add support for advertising FULLSYNC to Mac OSX Clients
Ralph Böhme
slow at samba.org
Mon Nov 21 16:00:26 UTC 2016
Hi Kevin,
sorry for taking so long to get to this.
First the boring part, review:
On Mon, Oct 31, 2016 at 11:51:49AM -0400, Kevin Anderson wrote:
> --- a/docs-xml/manpages/vfs_fruit.8.xml
> +++ b/docs-xml/manpages/vfs_fruit.8.xml
> @@ -108,6 +108,27 @@
> </varlistentry>
>
> <varlistentry>
> + <term>fruit:advertise_fullsync = [ true | false ]</term>
maybe we may want to call a spade a space and name this option
"fruit:time machine". Thoughts?
> + <listitem>
> + <para>Controls if the FULLSYNC volume capability is advertised to clients</para>
> +
> + <itemizedlist>
> + <listitem><para><command>false (default)</command>
> + Disables advertising the FULLSYNC volume capability to clients.
> + </para></listitem>
> +
> + <listitem><para><command>true</command> - Enables advertising
> + the FULLSYNC volume capability to clients. This is necessary for supporting
> + Time Machine backups from Mac OSX clients. This value only advertises the
> + capability and does nothing else with the sync requests from clients.
> + </para></listitem>
> +
> + </itemizedlist>
> +
> + </listitem>
> + </varlistentry>
> +
> + <varlistentry>
> <term>fruit:metadata = [ stream | netatalk ]</term>
> <listitem>
> <para>Controls where the OS X metadata stream is stored:</para>
> diff --git a/libcli/smb/smb2_create_ctx.h b/libcli/smb/smb2_create_ctx.h
> index cb194f5..1c65e6c 100644
> --- a/libcli/smb/smb2_create_ctx.h
> +++ b/libcli/smb/smb2_create_ctx.h
> @@ -30,7 +30,7 @@
>
> /* "AAPL" Server Query request/response bitmap */
> #define SMB2_CRTCTX_AAPL_SERVER_CAPS 1
> -#define SMB2_CRTCTX_AAPL_VOLUME_CAPS 2
> +#define SMB2_CRTCTX_AAPL_VOLUME_CAPS 6
> #define SMB2_CRTCTX_AAPL_MODEL_INFO 4
This definitely looks wrong, these are the defines for individual bits
in a bitfield. Why are you changing SMB2_CRTCTX_AAPL_VOLUME_CAPS to 6 ?
>
> /* "AAPL" Client/Server Capabilities bitmap */
> @@ -42,5 +42,6 @@
> /* "AAPL" Volume Capabilities bitmap */
> #define SMB2_CRTCTX_AAPL_SUPPORT_RESOLVE_ID 1
> #define SMB2_CRTCTX_AAPL_CASE_SENSITIVE 2
> +#define SMB2_CRTCTX_AAPL_FULL_SYNC 4
>
> #endif
> diff --git a/source3/modules/vfs_fruit.c b/source3/modules/vfs_fruit.c
> index 6e7899aa..c0101c1 100644
> --- a/source3/modules/vfs_fruit.c
> +++ b/source3/modules/vfs_fruit.c
> @@ -133,6 +133,7 @@ struct fruit_config_data {
> bool copyfile_enabled;
> bool veto_appledouble;
> bool posix_rename;
> + bool advertise_fullsync;
>
> /*
> * Additional options, all enabled by default,
> @@ -1352,6 +1353,9 @@ static int init_fruit_config(vfs_handle_struct *handle)
> config->use_aapl = lp_parm_bool(
> -1, FRUIT_PARAM_TYPE_NAME, "aapl", true);
>
> + config->advertise_fullsync = lp_parm_bool(
> + -1, FRUIT_PARAM_TYPE_NAME, "advertise_fullsync", false);
config->advertise_fullsync = lp_parm_bool(
-1, FRUIT_PARAM_TYPE_NAME, "time machine", false);
> config->unix_info_enabled = lp_parm_bool(
> -1, FRUIT_PARAM_TYPE_NAME, "nfs_aces", true);
>
> @@ -1827,6 +1831,7 @@ static NTSTATUS check_aapl(vfs_handle_struct *handle,
> DATA_BLOB blob = data_blob_talloc(req, NULL, 0);
> uint64_t req_bitmap, client_caps;
> uint64_t server_caps = SMB2_CRTCTX_AAPL_UNIX_BASED;
> + uint64_t volume_caps;
> smb_ucs2_t *model;
> size_t modellen;
>
> @@ -1898,7 +1903,11 @@ static NTSTATUS check_aapl(vfs_handle_struct *handle,
> if (req_bitmap & SMB2_CRTCTX_AAPL_VOLUME_CAPS) {
> SBVAL(p, 0,
> lp_case_sensitive(SNUM(handle->conn->tcon->compat)) ?
> - SMB2_CRTCTX_AAPL_CASE_SENSITIVE : 0);
> + volume_caps |= SMB2_CRTCTX_AAPL_CASE_SENSITIVE : 0);
> +
> + if (config->advertise_fullsync) {
> + SBVAL(p, 0, volume_caps |= SMB2_CRTCTX_AAPL_FULL_SYNC);
> + }
> ok = data_blob_append(req, &blob, p, 8);
> if (!ok) {
> return NT_STATUS_UNSUCCESSFUL;
>
> From bdd62adfc29f768cca5ecb6fb2d4bad2c6b0cf35 Mon Sep 17 00:00:00 2001
> From: Kevin Anderson <andersonkw2 at gmail.com>
> Date: Tue, 25 Oct 2016 16:21:19 -0400
> Subject: [PATCH 2/2] vfs_fruit: Add/Fix config option to enable/disable
> fullsync
>
> Add a configuration option to disable/enable fullsync
> and to fix mistakenly overwriting values incorrectly.
Cleaned up in the attached patchset.
I've also added code that ensures all prerequisite Samba options are
set on the fly when a Time Machine enabled share is connected.
Now, secondly, the interesting part: have you ever tested if the TM
disk image filesystem survives network disconnects and/or hard server
power offs ?
I've been told that there seems to be an issue in the Linux kernel not
properly flushing buffers to disk in an fsync() resulting in damaged
TM disk image filesytems. This was discovered by folks running tests
with a similar patch.
From hearsay, some storage devices cheet when they get a flush
write-buffer command and ignore it, but the testing was done with a
storage device that was known not to cheet. But still, after power
cycling the server while a TM backup was in progress the TM disk image
filesystem was frequently reported as damaged by the client.
Do we want to put our users at risk of loosing their backups in
situations like this ? Do we want to pretend being a suitable backup
target for something that breaks easily for unknown reasons ?
From past experience of adding Time Machine support to another
filesharing protocol (for AFP/Netatalk), I'm worried that our
mailing lists will be flooded with complaints like this:
<https://www.google.de/search?q=time+machine+must+create+a+new+backup+for+you>
It seems just putting your laptop to sleep or disconnecting from
network while TM is running seems to be the primary cause for this. To
me it's entirely unclear how this relates to fsync implementation bugs,
it might be unrelated.
I'm not saying that I'm really opposed to adding the patch, sooner
then later vendors will stick it in their boxes anyway, but I'd like
to stir up some discussion and raise awareness.
Thoughts?
Cheerio!
-slow
-------------- next part --------------
From 9b94c459a6d1f491cb85c105174c197e812fa51c Mon Sep 17 00:00:00 2001
From: Kevin Anderson <andersonkw2 at gmail.com>
Date: Mon, 14 Nov 2016 19:14:44 +0100
Subject: [PATCH 1/2] vfs_fruit: Add Time Machine support
Add a configuration option to disable/enable Time Machine support via
the FULLSYNC AAPL flag.
---
source3/modules/vfs_fruit.c | 25 ++++++++++++++++++++++---
1 file changed, 22 insertions(+), 3 deletions(-)
diff --git a/source3/modules/vfs_fruit.c b/source3/modules/vfs_fruit.c
index 6e7899aa..9304875 100644
--- a/source3/modules/vfs_fruit.c
+++ b/source3/modules/vfs_fruit.c
@@ -133,6 +133,7 @@ struct fruit_config_data {
bool copyfile_enabled;
bool veto_appledouble;
bool posix_rename;
+ bool time_machine;
/*
* Additional options, all enabled by default,
@@ -1352,6 +1353,9 @@ static int init_fruit_config(vfs_handle_struct *handle)
config->use_aapl = lp_parm_bool(
-1, FRUIT_PARAM_TYPE_NAME, "aapl", true);
+ config->time_machine = lp_parm_bool(
+ -1, FRUIT_PARAM_TYPE_NAME, "time machine", false);
+
config->unix_info_enabled = lp_parm_bool(
-1, FRUIT_PARAM_TYPE_NAME, "nfs_aces", true);
@@ -1827,6 +1831,7 @@ static NTSTATUS check_aapl(vfs_handle_struct *handle,
DATA_BLOB blob = data_blob_talloc(req, NULL, 0);
uint64_t req_bitmap, client_caps;
uint64_t server_caps = SMB2_CRTCTX_AAPL_UNIX_BASED;
+ uint64_t volume_caps = 0;
smb_ucs2_t *model;
size_t modellen;
@@ -1896,9 +1901,16 @@ static NTSTATUS check_aapl(vfs_handle_struct *handle,
}
if (req_bitmap & SMB2_CRTCTX_AAPL_VOLUME_CAPS) {
- SBVAL(p, 0,
- lp_case_sensitive(SNUM(handle->conn->tcon->compat)) ?
- SMB2_CRTCTX_AAPL_CASE_SENSITIVE : 0);
+ if (lp_case_sensitive(SNUM(handle->conn->tcon->compat))) {
+ volume_caps |= SMB2_CRTCTX_AAPL_CASE_SENSITIVE;
+ }
+
+ if (config->time_machine) {
+ volume_caps |= SMB2_CRTCTX_AAPL_FULL_SYNC;
+ }
+
+ SBVAL(p, 0, volume_caps);
+
ok = data_blob_append(req, &blob, p, 8);
if (!ok) {
return NT_STATUS_UNSUCCESSFUL;
@@ -2113,6 +2125,13 @@ static int fruit_connect(vfs_handle_struct *handle,
"0x0d:0xf00d");
}
+ if (config->time_machine) {
+ lp_do_parameter(SNUM(handle->conn), "durable handles", "yes");
+ lp_do_parameter(SNUM(handle->conn), "strict sync", "yes");
+ lp_do_parameter(SNUM(handle->conn), "kernel oplocks", "no");
+ lp_do_parameter(SNUM(handle->conn), "kernel share modes", "no");
+ lp_do_parameter(SNUM(handle->conn), "posix locking", "no");
+ }
return rc;
}
--
2.7.4
From 40ebbbfe761bc96775ab76c8bf49d0a15852cf50 Mon Sep 17 00:00:00 2001
From: Kevin Anderson <andersonkw2 at gmail.com>
Date: Sun, 23 Oct 2016 20:32:27 -0400
Subject: [PATCH 2/2] docs/vfs_fruit: Add Time Machine support
Add the capability to advertise FULLSYNC volume capabilities
to clients that request them. This is mainly used for supporting
Mac OS Time Machine backups from clients. The capability does
not perform any additional action.
---
docs-xml/manpages/vfs_fruit.8.xml | 33 +++++++++++++++++++++++++++++++++
libcli/smb/smb2_create_ctx.h | 1 +
2 files changed, 34 insertions(+)
diff --git a/docs-xml/manpages/vfs_fruit.8.xml b/docs-xml/manpages/vfs_fruit.8.xml
index 7a84f87..ce9e7c9 100644
--- a/docs-xml/manpages/vfs_fruit.8.xml
+++ b/docs-xml/manpages/vfs_fruit.8.xml
@@ -165,6 +165,39 @@
</varlistentry>
<varlistentry>
+ <term>fruit:time machine = [ yes | no ]</term>
+ <listitem>
+ <para>Controls if Time Machine support via the FULLSYNC volume
+ capability is advertised to clients.</para>
+
+ <itemizedlist>
+ <listitem><para><command>no (default)</command> Disables
+ advertising Time Machine support and the FULLSYNC volume
+ capability to clients.</para></listitem>
+
+ <listitem><para><command>yes</command> - Enables advertising
+ Time Machine support and the FULLSYNC volume capability to
+ clients. This is necessary for supporting Time Machine backups
+ from Mac OSX clients. This value only advertises the capability
+ and does nothing else with the sync requests from clients.
+ </para></listitem>
+
+ </itemizedlist>
+
+ <para>This option enforces the following settings per share (or
+ for all shares if enabled globally):</para>
+ <itemizedlist>
+ <listitem><para><command>durable handles = yes</command></para></listitem>
+ <listitem><para><command>kernel oplocks = no</command></para></listitem>
+ <listitem><para><command>kernel share modes = no</command></para></listitem>
+ <listitem><para><command>posix locking = no</command></para></listitem>
+ <listitem><para><command>strict sync = yes</command></para></listitem>
+ </itemizedlist>
+
+ </listitem>
+ </varlistentry>
+
+ <varlistentry>
<term>fruit:aapl = yes | no</term>
<listitem>
<para>A global option whether to enable Apple's SMB2+
diff --git a/libcli/smb/smb2_create_ctx.h b/libcli/smb/smb2_create_ctx.h
index cb194f5..0e0b713 100644
--- a/libcli/smb/smb2_create_ctx.h
+++ b/libcli/smb/smb2_create_ctx.h
@@ -42,5 +42,6 @@
/* "AAPL" Volume Capabilities bitmap */
#define SMB2_CRTCTX_AAPL_SUPPORT_RESOLVE_ID 1
#define SMB2_CRTCTX_AAPL_CASE_SENSITIVE 2
+#define SMB2_CRTCTX_AAPL_FULL_SYNC 4
#endif
--
2.7.4
More information about the samba-technical
mailing list