[Patch] dosmode: Compare block devices to detect reparse points
Jeremy Allison
jra at samba.org
Tue Dec 4 17:51:37 UTC 2018
On Tue, Dec 04, 2018 at 02:48:17PM +0000, Noel Power via samba-technical wrote:
> Hi All,
>
> So coincidently I also have been looking at this recently due to a couple of
> our customers coming across this problem, unfortunately I have been tied up
> a bit with the python3 stuff :-) I meant to post about it but of course
> things have got in the way.
> On 03/12/2018 20:22, Jeremy Allison via samba-technical wrote:
> > On Mon, Dec 03, 2018 at 01:11:38PM -0700, James Ashdown via samba-technical wrote:
> > > This patch addresses the problem reported in Bug 13122
> > > <https://bugzilla.samba.org/show_bug.cgi?id=13122>, which describes write
> > > problems when a file share contains a mountpoint or a wide link that points
> > > to another volume.
> > >
> > > Currently, the smbd response always returns 'free units' for the volume
> > > containing the root of the share. If the root volume is full and the write
> > > destination is on a different volume, the client refuses to write because
> > > the FsFullSizeInfo states that the disk is full.
> > >
> > > If the root volume has sufficient space and the write destination does not,
> > > the write will proceed and truncate.
> > >
> > > Flagging each volume crossing (mountpoints and wide links) as reparse points
> > > enables correct quota reporting for the actual write destination. The flag
> > > is set by checking whether each directory has a different device ID (using
> > > smb_filename->st.st_ex_dev) than its parent directory.
> > This is adding a bunch of expensive syscalls in a hot code
> > path, so for that reason alone this should be an option,
> > not the default.
>
> Absolutely, this is similar to how DFS is handled so it needs to be
> optional, also I think having this in dosmode as is in this patch is
> probably overkill
>
> It seems that only in the result of SMB2_FIND_ID_BOTH_DIRECTORY_INFO do we
> need to set FILE_ATTRIBUTE_REPARSE_POINT for entries that are 'mount points'
> Additionally the response flags for in the SMB2 create response should be
> set to ReparsePoint (although iirc in my investigations this didn't seem to
> make a difference)
My problem with setting FILE_ATTRIBUTE_REPARSE_POINT on any return
is that smbd doesn't handle these correctly.
We set it on MS-DFS links because we handle them the right way
in smbd on client query.
In my SMB2-POSIX branch we set FILE_ATTRIBUTE_REPARSE_POINT on
POSIX symlinks/fifos/block/character devices because I've added
the code to smbd in that branch to correctly handle the pathname
queries on names we return with this.
But in general we simply ignore the FILE_OPEN_REPARSE_POINT
requests in smbd. git grep for it and you'll see.
I'm really averse to returning FILE_ATTRIBUTE_REPARSE_POINT
on anything just to get different client behavior, when we
don't handle FILE_OPEN_REPARSE_POINT in any way in smbd.
That's the core of my objections to these things.
Jeremy.
> > Secondly, what effects does returning (FILE_ATTRIBUTE_REPARSE_POINT|existing_mode)
> > have on the client ?
>
> So probably best first to tell what the current problem is,
>
> If you have a SAMBA share (where the directory is on device /dev/deviceA)
> and somewhere in the the tree of children you have a folder which is a
> mountpoint to a directory on a different device (e.g. /dev/deviceB) then the
> following happens
>
> a) The quotas associated with /dev/deviceA are also applied to the folders
> that are located on /dev/deviceB, very annoying if the quota on /dev/deviceB
> hasn't been exceeded and you should still be able to write stuff to this
> device but the quota has been reached on /dev/deviceA so you can't :-)
>
> b) The free space capacity of /dev/deviceA is used throughout the share
> (including those folders located on a difference device) this is of course
> also annoying if the capacity of /dev/deviceA is smaller (or has no space
> left) and there is plenty of space on /dev/deviceB meaning you cannot write
> to folders that are on /dev/deviceB even though there is plenty of free
> capacity
>
> c) Worse is to the customer this appears be a regression because the
> behaviour is only seen when you switch from SMB1 to >=SMB2. With SMB1 the
> client doesn't seem to take much notice of whatever information it may have
> about capacities or quotas but relies on the server to inform when it is no
> longer possible to write (due to quota exceeded or disk full) With SMB2 the
> window client takes the information it has (about quotas & capacity) and it
> decides not to write to the disk if it determines that there is not enough
> space to perform say a copy (e.g. the server doesn't even get told to
> attempt the write or copy)
>
> What effect does this have on the client ? So for the tests I have done
> (with attached patch) results in the windows client issuing
> FS_INFO/FileFsFullSizeInformation requests for the mount point directory as
> needed and the capactity quota settings being honoured for both file
> systems.
>
> Note: windows client == explorer in this case (and windows 8.1 specifically)
>
> This problem is appearing more and more often as customers are transitioning
> from SMB1 -> SMB2+
>
>
> >
> > Thirdly, isn't this a problem that could be solved by a combination
> > of (a) Don't do that, and (b) Use MS-DFS redirects ?
> sure but customers don't want to change working setups and scream it was
> working before, this is a regression blah blah balh. Also windows supports
> this scenario with 'mount points' a special type of reparse point
> > I know Samba is convenient for stitching together arbitrary
> > filesystems into one convenient point for clients to mount,
> > but this kind of thing seems to me to be something that the
> > Admin should be thinking about and taking care of in advance.
> >
> > Jeremy.
> >
> >
> so, like I said I was also looking into this (didn't know about the
> associated https://bugzilla.samba.org/show_bug.cgi?id=13122) which mentions
> some problems also with Mac.
>
> I think it would be useful to support this as a optional share setting (like
> I was already doing) does this make it more palatable Jeremy ? I was
> intending trying to do some more testing around this anyway (I only tested
> one windows client version) and possibly also extending smbclient allinfo to
> take into account reparse_point(s) (currently only symlinks are handled)
>
>
> Noel
>
> From 7edd85e113af37d79db5b3f28fb0c634069e6b92 Mon Sep 17 00:00:00 2001
> From: Noel Power <noel.power at suse.com>
> Date: Fri, 2 Nov 2018 14:56:12 +0000
> Subject: [PATCH 1/4] s3/smbd: Simple function to detect a mountpoint
>
> ---
> source3/include/mountpoint.h | 27 ++++++++++++++++
> source3/smbd/mountpoint.c | 76 ++++++++++++++++++++++++++++++++++++++++++++
> source3/wscript_build | 1 +
> 3 files changed, 104 insertions(+)
> create mode 100644 source3/include/mountpoint.h
> create mode 100644 source3/smbd/mountpoint.c
>
> diff --git a/source3/include/mountpoint.h b/source3/include/mountpoint.h
> new file mode 100644
> index 0000000..3e78a90
> --- /dev/null
> +++ b/source3/include/mountpoint.h
> @@ -0,0 +1,27 @@
> +/*
> + Unix SMB/Netbios implementation.
> + Version 3.0
> + mountpoint support for Samba
> +
> + This program is free software; you can redistribute it and/or modify
> + it under the terms of the GNU General Public License as published by
> + the Free Software Foundation; either version 3 of the License, or
> + (at your option) any later version.
> +
> + This program is distributed in the hope that it will be useful,
> + but WITHOUT ANY WARRANTY; without even the implied warranty of
> + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + GNU General Public License for more details.
> +
> + You should have received a copy of the GNU General Public License
> + along with this program. If not, see <http://www.gnu.org/licenses/>.
> +
> +*/
> +
> +#ifndef _MOUNTPOINT_H
> +#define _MOUNTPOINT_H
> +#include "vfs.h"
> +
> +bool is_mountpoint(connection_struct *conn,
> + struct smb_filename *smb_fname);
> +#endif /* _MOUNTPOINT_H */
> diff --git a/source3/smbd/mountpoint.c b/source3/smbd/mountpoint.c
> new file mode 100644
> index 0000000..fee3925
> --- /dev/null
> +++ b/source3/smbd/mountpoint.c
> @@ -0,0 +1,76 @@
> +/*
> + Unix SMB/Netbios implementation.
> + Version 3.0
> + Mountpoint support for Samba
> +
> + This program is free software; you can redistribute it and/or modify
> + it under the terms of the GNU General Public License as published by
> + the Free Software Foundation; either version 3 of the License, or
> + (at your option) any later version.
> +
> + This program is distributed in the hope that it will be useful,
> + but WITHOUT ANY WARRANTY; without even the implied warranty of
> + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + GNU General Public License for more details.
> +
> + You should have received a copy of the GNU General Public License
> + along with this program. If not, see <http://www.gnu.org/licenses/>.
> +
> +*/
> +
> +#include "includes.h"
> +#include "mountpoint.h"
> +
> +/*
> + * Detects if smb_fname is a mount point.
> + * Returns true if device associated with smb_fname and it's
> + * parent directory differ.
> + */
> +bool is_mountpoint(connection_struct *conn,
> + struct smb_filename *smb_fname)
> +{
> + bool is_dir = (S_ISDIR(smb_fname->st.st_ex_mode));
> + char *parent = NULL;
> + bool mountpoint = false;
> + /* #TODO add a share config to enable/disable this feature */
> + if (is_dir) {
> + struct smb_filename *smb_fname_parent = NULL;
> + dev_t* pparent_dev;
> + dev_t* pchild_dev;
> + if (!parent_dirname(talloc_tos(), smb_fname->base_name,
> + &parent, NULL)) {
> + DBG_ERR("## failed to get parent for %s\n",
> + smb_fname->base_name);
> + goto out;
> + }
> + smb_fname_parent = synthetic_smb_fname(
> + talloc_tos(), parent,
> + NULL, NULL);
> + if (smb_fname_parent == NULL) {
> + DBG_ERR("## failed to create synthetic_fname for %s\n",
> + smb_fname_parent->base_name);
> + goto out;
> + }
> + pparent_dev = &smb_fname_parent->st.st_ex_dev;
> + pchild_dev = &smb_fname->st.st_ex_dev;
> + if (SMB_VFS_STAT(conn, smb_fname_parent) != 0) {
> + DBG_ERR("## failed to vstat synthetic_fname %s\n",
> + smb_fname_parent->base_name);
> + goto out;
> + }
> + DBG_DEBUG("## checking child: %s:%s%s child mode 0x%x parent: %s "
> + "parent mode 0x%x child ex_dev %ld parent ex_dev %ld\n",
> + smb_fname->base_name, smb_fname_str_dbg(smb_fname),
> + is_dir ? " [dir]" : "",
> + smb_fname->st.st_ex_mode,
> + smb_fname_parent->base_name,
> + smb_fname_parent->st.st_ex_mode,
> + smb_fname->st.st_ex_dev,
> + smb_fname_parent->st.st_ex_dev);
> +
> + mountpoint = (*pparent_dev != *pchild_dev);
> + }
> +out:
> + TALLOC_FREE(parent);
> + return mountpoint;
> +}
> diff --git a/source3/wscript_build b/source3/wscript_build
> index 9033222..1a67b9e 100755
> --- a/source3/wscript_build
> +++ b/source3/wscript_build
> @@ -559,6 +559,7 @@ bld.SAMBA3_LIBRARY('smbd_base',
> smbd/quotas.c
> smbd/ntquotas.c
> smbd/msdfs.c
> + smbd/mountpoint.c
> smbd/aio.c smbd/statvfs.c
> smbd/dmapi.c
> smbd/signing.c
> --
> 2.12.3
>
>
> From d37c4b1189b65d7bcc900cad0b5662df432f7bf5 Mon Sep 17 00:00:00 2001
> From: Noel Power <noel.power at suse.com>
> Date: Fri, 2 Nov 2018 14:59:00 +0000
> Subject: [PATCH 2/4] s3/smbd: detect mountpoint and report for
> FIND_ID_DIRECTORY_BOTH_INFO
>
> Need to return the REPARSE_POINT file attribute in order for windows
> client to subsequently query for new capacity
> ---
> source3/smbd/trans2.c | 15 +++++++++++++++
> 1 file changed, 15 insertions(+)
>
> diff --git a/source3/smbd/trans2.c b/source3/smbd/trans2.c
> index 2149da3..3f76b04 100644
> --- a/source3/smbd/trans2.c
> +++ b/source3/smbd/trans2.c
> @@ -41,6 +41,7 @@
> #include "printing.h"
> #include "lib/util_ea.h"
> #include "lib/readdir_attr.h"
> +#include "source3/include/mountpoint.h"
>
> #define DIR_ENTRY_SAFETY_MARGIN 4096
>
> @@ -1651,6 +1652,20 @@ static bool smbd_dirptr_lanman2_mode_fn(TALLOC_CTX *ctx,
> mode = dos_mode_msdfs(state->conn, smb_fname);
> } else {
> mode = dos_mode(state->conn, smb_fname);
> + /*
> + * mount point only shows FILE_ATTRIBUTE_REPARSE_POINT
> + * in ID_DIR_BOTH_INFO, for some create requests
> + * we get REPARSE_POINT attribute set, othertimes we dont
> + * but in those cases it seems the response flags have the
> + * ReparsePoint bit set (seems based on the createoptions)
> + * But.. the point is that we probably don't want to add
> + * FILE_ATTRIBUTE_REPARSE_POINT in dos_mode
> + */
> + if (is_mountpoint(state->conn, smb_fname)) {
> + mode |= FILE_ATTRIBUTE_REPARSE_POINT;
> + }
> +
> +
> }
>
> *_mode = mode;
> --
> 2.12.3
>
>
> From 5b68ab55ecb4d9830c41d25426eccaef50b63f0c Mon Sep 17 00:00:00 2001
> From: Noel Power <noel.power at suse.com>
> Date: Fri, 2 Nov 2018 15:22:57 +0000
> Subject: [PATCH 3/4] docs-xmls: Add a share level config item to control
> mountpoint support
>
> ---
> docs-xml/smbdotconf/vfs/mountpoint.xml | 12 ++++++++++++
> 1 file changed, 12 insertions(+)
> create mode 100644 docs-xml/smbdotconf/vfs/mountpoint.xml
>
> diff --git a/docs-xml/smbdotconf/vfs/mountpoint.xml b/docs-xml/smbdotconf/vfs/mountpoint.xml
> new file mode 100644
> index 0000000..a6ee221
> --- /dev/null
> +++ b/docs-xml/smbdotconf/vfs/mountpoint.xml
> @@ -0,0 +1,12 @@
> +<samba:parameter name="mountpoint"
> + context="S"
> + type="boolean"
> + xmlns:samba="http://www.samba.org/samba/DTD/samba-doc">
> +<description>
> + <para>If set to <constant>yes</constant>, Samba treats the
> + a mounted directory within a share as a special type of
> + reparse point (a mountpoint). #TODO more blah blah.</para>
> +</description>
> +
> +<value type="default">no</value>
> +</samba:parameter>
> --
> 2.12.3
>
>
> From e1faae61dc8c42747833aa7a90de8b9cdacc372c Mon Sep 17 00:00:00 2001
> From: Noel Power <noel.power at suse.com>
> Date: Fri, 2 Nov 2018 16:26:59 +0000
> Subject: [PATCH 4/4] s3/smbd: Add support for share level config mountpoint
> support
>
> ---
> source3/smbd/mountpoint.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/source3/smbd/mountpoint.c b/source3/smbd/mountpoint.c
> index fee3925..0176ce9 100644
> --- a/source3/smbd/mountpoint.c
> +++ b/source3/smbd/mountpoint.c
> @@ -32,8 +32,8 @@ bool is_mountpoint(connection_struct *conn,
> bool is_dir = (S_ISDIR(smb_fname->st.st_ex_mode));
> char *parent = NULL;
> bool mountpoint = false;
> - /* #TODO add a share config to enable/disable this feature */
> - if (is_dir) {
> +
> + if (is_dir && lp_mountpoint(SNUM(conn))) {
> struct smb_filename *smb_fname_parent = NULL;
> dev_t* pparent_dev;
> dev_t* pchild_dev;
> --
> 2.12.3
>
More information about the samba-technical
mailing list