[PATCH] vfs_glusterfs: Add fallocate support for vfs_glusterfs

classic Classic list List threaded Threaded
10 messages Options
Reply | Threaded
Open this post in threaded view
|

[PATCH] vfs_glusterfs: Add fallocate support for vfs_glusterfs

Samba - samba-technical mailing list
Hello,

Attached patch adds fallocate support to the vfs glusterfs plugin.

Thanks,
Sachin Prabhu

vfs_glusterfs_fallocate.patch (2K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] vfs_glusterfs: Add fallocate support for vfs_glusterfs

Samba - samba-technical mailing list
Hi Sachin,

A couple of comments inline...

On Tue, 09 Jan 2018 13:02:13 +0530, Sachin Prabhu via samba-technical wrote:

> From 05f47588b2641242d219564e6eb8468f9484a3d4 Mon Sep 17 00:00:00 2001
> From: Sachin Prabhu <[hidden email]>
> Date: Tue, 14 Nov 2017 15:51:44 +0530
> Subject: [PATCH] vfs_glusterfs: Add fallocate support for vfs_glusterfs
>
> Adds fallocate support to the vfs glusterfs plugin.
>
> RHBZ: 1478875
>
> Signed-off-by: Sachin Prabhu <[hidden email]>
> ---
>  source3/modules/vfs_glusterfs.c | 26 ++++++++++++++++++++++----
>  1 file changed, 22 insertions(+), 4 deletions(-)
>
> diff --git a/source3/modules/vfs_glusterfs.c b/source3/modules/vfs_glusterfs.c
> index 953c46af4cd..c0ea722c1df 100644
> --- a/source3/modules/vfs_glusterfs.c
> +++ b/source3/modules/vfs_glusterfs.c
> @@ -462,7 +462,8 @@ static int vfs_gluster_statvfs(struct vfs_handle_struct *handle,
>  static uint32_t vfs_gluster_fs_capabilities(struct vfs_handle_struct *handle,
>      enum timestamp_set_resolution *p_ts_res)
>  {
> - uint32_t caps = FILE_CASE_SENSITIVE_SEARCH | FILE_CASE_PRESERVED_NAMES;
> + uint32_t caps = FILE_CASE_SENSITIVE_SEARCH | FILE_CASE_PRESERVED_NAMES
> + | FILE_SUPPORTS_SPARSE_FILES;

Does glusterfs support SEEK_DATA / SEEK_HOLE? Keep in mind that sparse
aware clients may issue Query Allocated Ranges requests, which can be
mapped by the SMB2 server into VFS lseek(SEEK_DATA / SEEK_HOLE) calls.

>  #ifdef STAT_HAVE_NSEC
>   *p_ts_res = TIMESTAMP_SET_NT_OR_BETTER;
> @@ -1148,9 +1149,26 @@ static int vfs_gluster_fallocate(struct vfs_handle_struct *handle,
>   uint32_t mode,
>   off_t offset, off_t len)
>  {
> - /* TODO: add support using glfs_fallocate() and glfs_zerofill() */
> - errno = ENOTSUP;
> - return -1;
> + int keep_size, punch_hole;
> +
> + keep_size = mode & VFS_FALLOCATE_FL_KEEP_SIZE;
> + punch_hole = mode & VFS_FALLOCATE_FL_PUNCH_HOLE;
> +
> + mode &= ~(VFS_FALLOCATE_FL_KEEP_SIZE|VFS_FALLOCATE_FL_PUNCH_HOLE);
> + if (mode) {
> + errno = ENOTSUP;
> + return -1;
> + }
> +
> + if (punch_hole) {
> + return glfs_discard(*(glfs_fd_t **)
> +    VFS_FETCH_FSP_EXTENSION(handle, fsp),
> +    offset, len);

I assume glfs_discard() will never change the file size here, i.e. it
matches fallocate(mode = VFS_FALLOCATE_FL_PUNCH_HOLE
                        | VFS_FALLOCATE_FL_KEEP_SIZE) behaviour?
> + }
> +
> + return glfs_fallocate(*(glfs_fd_t **)
> +      VFS_FETCH_FSP_EXTENSION(handle, fsp),
> +      keep_size, offset, len);
>  }
>  
>  static struct smb_filename *vfs_gluster_realpath(struct vfs_handle_struct *handle,

Cheers, David

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] vfs_glusterfs: Add fallocate support for vfs_glusterfs

Samba - samba-technical mailing list
On Tue, 2018-01-09 at 23:56 +0100, David Disseldorp via samba-technical
wrote:
> Hi Sachin,
>
> A couple of comments inline...
Hello David,

Replies inline as well.


>
> On Tue, 09 Jan 2018 13:02:13 +0530, Sachin Prabhu via samba-technical
> wrote:
>
> > From 05f47588b2641242d219564e6eb8468f9484a3d4 Mon Sep 17 00:00:00
> > 2001
> > From: Sachin Prabhu <[hidden email]>
> > Date: Tue, 14 Nov 2017 15:51:44 +0530
> > Subject: [PATCH] vfs_glusterfs: Add fallocate support for
> > vfs_glusterfs
> >
> > Adds fallocate support to the vfs glusterfs plugin.
> >
> > RHBZ: 1478875
> >
> > Signed-off-by: Sachin Prabhu <[hidden email]>
> > ---
> >  source3/modules/vfs_glusterfs.c | 26 ++++++++++++++++++++++----
> >  1 file changed, 22 insertions(+), 4 deletions(-)
> >
> > diff --git a/source3/modules/vfs_glusterfs.c
> > b/source3/modules/vfs_glusterfs.c
> > index 953c46af4cd..c0ea722c1df 100644
> > --- a/source3/modules/vfs_glusterfs.c
> > +++ b/source3/modules/vfs_glusterfs.c
> > @@ -462,7 +462,8 @@ static int vfs_gluster_statvfs(struct
> > vfs_handle_struct *handle,
> >  static uint32_t vfs_gluster_fs_capabilities(struct
> > vfs_handle_struct *handle,
> >      enum
> > timestamp_set_resolution *p_ts_res)
> >  {
> > - uint32_t caps = FILE_CASE_SENSITIVE_SEARCH |
> > FILE_CASE_PRESERVED_NAMES;
> > + uint32_t caps = FILE_CASE_SENSITIVE_SEARCH |
> > FILE_CASE_PRESERVED_NAMES
> > + | FILE_SUPPORTS_SPARSE_FILES;
>
> Does glusterfs support SEEK_DATA / SEEK_HOLE? Keep in mind that
> sparse
> aware clients may issue Query Allocated Ranges requests, which can be
> mapped by the SMB2 server into VFS lseek(SEEK_DATA / SEEK_HOLE)
> calls.

Yes they do.

I have used the smbtorture test smb2.ioctl.sparse_punch to test this
patch. This test makes the ioctl call - Query allocated ranges.

I have also run tests outside the samba environment to confirm that
glfs_lseek() does support SEEK_DATA and SEEK_HOLE.

>
> >  #ifdef STAT_HAVE_NSEC
> >   *p_ts_res = TIMESTAMP_SET_NT_OR_BETTER;
> > @@ -1148,9 +1149,26 @@ static int vfs_gluster_fallocate(struct
> > vfs_handle_struct *handle,
> >   uint32_t mode,
> >   off_t offset, off_t len)
> >  {
> > - /* TODO: add support using glfs_fallocate() and
> > glfs_zerofill() */
> > - errno = ENOTSUP;
> > - return -1;
> > + int keep_size, punch_hole;
> > +
> > + keep_size = mode & VFS_FALLOCATE_FL_KEEP_SIZE;
> > + punch_hole = mode & VFS_FALLOCATE_FL_PUNCH_HOLE;
> > +
> > + mode &=
> > ~(VFS_FALLOCATE_FL_KEEP_SIZE|VFS_FALLOCATE_FL_PUNCH_HOLE);
> > + if (mode) {
> > + errno = ENOTSUP;
> > + return -1;
> > + }
> > +
> > + if (punch_hole) {
> > + return glfs_discard(*(glfs_fd_t **)
> > +    VFS_FETCH_FSP_EXTENSION(handle
> > , fsp),
> > +    offset, len);
>
> I assume glfs_discard() will never change the file size here, i.e. it
> matches fallocate(mode = VFS_FALLOCATE_FL_PUNCH_HOLE
> | VFS_FALLOCATE_FL_KEEP_SIZE) behaviour?

Yes. The glfs_discard() on the client eventually ends up calling the
fallocate() syscall on the server with the flags set to
FALLOC_FL_KEEP_SIZE|FALLOC_FL_PUNCH_HOLE.

ie. on the server, the posix translator calls the syscall fallocate()
with the flags as mentioned.
static int32_t
posix_discard(call_frame_t *frame, xlator_t *this, fd_t *fd, off_t
offset,
              size_t len, dict_t *xdata)
{
..
        int32_t flags = FALLOC_FL_KEEP_SIZE|FALLOC_FL_PUNCH_HOLE;
..
        ret = posix_do_fallocate (frame, this, fd, flags, offset, len,
                                  &statpre, &statpost, xdata);
..
}


> > + }
> > +
> > + return glfs_fallocate(*(glfs_fd_t **)
> > +      VFS_FETCH_FSP_EXTENSION(handle,
> > fsp),
> > +      keep_size, offset, len);
> >  }
> >  
> >  static struct smb_filename *vfs_gluster_realpath(struct
> > vfs_handle_struct *handle,
>
> Cheers, David
>

Thanks for reviewing the patch.

Sachin Prabhu


Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] vfs_glusterfs: Add fallocate support for vfs_glusterfs

Samba - samba-technical mailing list
On Wed, 10 Jan 2018 15:41:27 +0530, Sachin Prabhu wrote:

> Hello David,
>
> Replies inline as well.

Thanks for your analysis here. I'm happy with the change...
Reviewed-by: David Disseldorp <[hidden email]>

Could someone please push this - @Michael / G√ľnther?

Cheers, David

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] vfs_glusterfs: Add fallocate support for vfs_glusterfs

Samba - samba-technical mailing list
In reply to this post by Samba - samba-technical mailing list
Hi Sachin,

Unfortunately this won't compile on an older
version of Gluster - it doesn't seem to have
glfs_discard() or glfs_fallocate().

What version of gluster were these added to ?

I think you might also need to update this
code in source3/wscript that detects the
version of the glusterfs-api.

    if Options.options.with_glusterfs:
        conf.CHECK_CFG(package='glusterfs-api', args='"glusterfs-api >= 4" --cflags --libs',
                       msg='Checking for glusterfs-api >= 4', uselib_store="GFAPI")
        conf.CHECK_HEADERS('glusterfs/api/glfs.h', lib='gfapi')
        conf.CHECK_LIB('gfapi', shlib=True)

Jeremy.

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] vfs_glusterfs: Add fallocate support for vfs_glusterfs

Samba - samba-technical mailing list
On 2018-01-10 at 12:34 -0800, Jeremy Allison via samba-technical wrote:
> Hi Sachin,
>
> Unfortunately this won't compile on an older
> version of Gluster - it doesn't seem to have
> glfs_discard() or glfs_fallocate().

Great point, thanks for catching this, Jeremy!

Waiting for a patch update to address this.
Otherwise the patch looks good to me.

Cheers - Michael

> What version of gluster were these added to ?
>
> I think you might also need to update this
> code in source3/wscript that detects the
> version of the glusterfs-api.
>
>     if Options.options.with_glusterfs:
>         conf.CHECK_CFG(package='glusterfs-api', args='"glusterfs-api >= 4" --cflags --libs',
>                        msg='Checking for glusterfs-api >= 4', uselib_store="GFAPI")
>         conf.CHECK_HEADERS('glusterfs/api/glfs.h', lib='gfapi')
>         conf.CHECK_LIB('gfapi', shlib=True)
>
> Jeremy.
>

signature.asc (169 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] vfs_glusterfs: Add fallocate support for vfs_glusterfs

Samba - samba-technical mailing list
In reply to this post by Samba - samba-technical mailing list
On Wed, 2018-01-10 at 12:34 -0800, Jeremy Allison wrote:

> Hi Sachin,
>
> Unfortunately this won't compile on an older
> version of Gluster - it doesn't seem to have
> glfs_discard() or glfs_fallocate().
>
> What version of gluster were these added to ?
>
> I think you might also need to update this
> code in source3/wscript that detects the
> version of the glusterfs-api.
>
>     if Options.options.with_glusterfs:
>         conf.CHECK_CFG(package='glusterfs-api', args='"glusterfs-api
> >= 4" --cflags --libs',
>                        msg='Checking for glusterfs-api >= 4',
> uselib_store="GFAPI")
>         conf.CHECK_HEADERS('glusterfs/api/glfs.h', lib='gfapi')
>         conf.CHECK_LIB('gfapi', shlib=True)
>
> Jeremy.


Hello Jeremy,

Thanks for pointing this out. These functions seem to be added in
version 3.5.0. I'll get back to the list on this issue.

Sachin Prabhu

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v2] vfs_glusterfs: Add fallocate support for vfs_glusterfs

Samba - samba-technical mailing list
In reply to this post by Samba - samba-technical mailing list
On Tue, 2018-01-09 at 13:02 +0530, Sachin Prabhu wrote:
> Hello,
>
> Attached patch adds fallocate support to the vfs glusterfs plugin.
>
> Thanks,
> Sachin Prabhu

New version of the patch which adds a check for glusterfs-api version
and enabled the new functions only if the api version > 6 ie. contains
support for glfs_fallocate() and glfs_discard().

Thanks,
Sachin Prabhu

0001-vfs_glusterfs-Add-fallocate-support-for-vfs_glusterf.patch (3K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v2] vfs_glusterfs: Add fallocate support for vfs_glusterfs

Samba - samba-technical mailing list
On Fri, 12 Jan 2018 18:03:59 +0530, Sachin Prabhu wrote:

> New version of the patch which adds a check for glusterfs-api version
> and enabled the new functions only if the api version > 6 ie. contains
> support for glfs_fallocate() and glfs_discard().

Looks good.
Reviewed-by: David Disseldorp <[hidden email]>

Cheers, David

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v2] vfs_glusterfs: Add fallocate support for vfs_glusterfs

Samba - samba-technical mailing list
On Fri, Jan 12, 2018 at 02:06:34PM +0100, David Disseldorp wrote:
> On Fri, 12 Jan 2018 18:03:59 +0530, Sachin Prabhu wrote:
>
> > New version of the patch which adds a check for glusterfs-api version
> > and enabled the new functions only if the api version > 6 ie. contains
> > support for glfs_fallocate() and glfs_discard().
>
> Looks good.
> Reviewed-by: David Disseldorp <[hidden email]>

Reviewed-by: Jeremy Allison <[hidden email]>

Will push once autobuild is working again.