[PATCH] vfs_gpfs: move btime stuff from stat to get_dos_attrs
Christof Schmitt
cs at samba.org
Thu Dec 15 19:40:22 UTC 2016
On Thu, Dec 15, 2016 at 08:14:57PM +0100, Ralph Böhme wrote:
> On Thu, Dec 15, 2016 at 11:47:08AM -0700, Christof Schmitt wrote:
> > On Thu, Dec 15, 2016 at 06:21:17PM +0100, Ralph Böhme wrote:
> > > Hi!
> > >
> > > Attached is a patch that addresses a performance problem in vfs_gpfs:
> > >
> > > perf report found that in a kernel copy workload smbd was spending
> > > considerable CPU time in vfs_gpfs_(f|l)stat -> gpfs_get_winattrs.
> > >
> > > Most of the time the VFS stat caller is not interested in the btime. The
> > > SMB frontend processing around btime is designed to fetch btime together
> > > with DOS attributes via dos_mode() in all places that need these
> > > attributes. That's the way it is implemented in the default VFS module
> > > and that's what vfs_gpfs now does as well for performance reasons.
> > >
> > > Please review and push if ok. Thanks!
> >
> > I like the approach of aligning with the common code and removing some
> > complexity from vfs_gpfs.
> >
> > To compile the module, these changes were necessary on top of your patch:
> >
> > diff --git a/source3/modules/vfs_gpfs.c b/source3/modules/vfs_gpfs.c
> > index 32dc527..f7434c9 100644
> > --- a/source3/modules/vfs_gpfs.c
> > +++ b/source3/modules/vfs_gpfs.c
> > @@ -1565,9 +1565,9 @@ static NTSTATUS vfs_gpfs_get_dos_attributes(struct vfs_handle_struct *handle,
> > }
> >
> > *dosmode |= vfs_gpfs_winattrs_to_dosmode(attrs.winAttrs);
> > - fsp->fsp_name->st.st_ex_calculated_birthtime = false;
> > - fsp->fsp_name->st.st_ex_btime.tv_sec = attrs.creationTime.tv_sec;
> > - fsp->fsp_name->st.st_ex_btime.tv_nsec = attrs.creationTime.tv_nsec;
> > + smb_fname->st.st_ex_calculated_birthtime = false;
> > + smb_fname->st.st_ex_btime.tv_sec = attrs.creationTime.tv_sec;
> > + smb_fname->st.st_ex_btime.tv_nsec = attrs.creationTime.tv_nsec;
> >
> > return NT_STATUS_OK;
> > }
> > @@ -1600,9 +1600,9 @@ static NTSTATUS vfs_gpfs_fget_dos_attributes(struct vfs_handle_struct *handle,
> > }
> >
> > *dosmode |= vfs_gpfs_winattrs_to_dosmode(attrs.winAttrs);
> > - smb_fname->st.st_ex_calculated_birthtime = false;
> > - smb_fname->st.st_ex_btime.tv_sec = attrs.creationTime.tv_sec;
> > - smb_fname->st.st_ex_btime.tv_nsec = attrs.creationTime.tv_nsec;
> > + fsp->fsp_name->st.st_ex_calculated_birthtime = false;
> > + fsp->fsp_name->st.st_ex_btime.tv_sec = attrs.creationTime.tv_sec;
> > + fsp->fsp_name->st.st_ex_btime.tv_nsec = attrs.creationTime.tv_nsec;
> >
> > return NT_STATUS_OK;
> > }
> >
> > Did you post a previous version of the patch by chance instead of the final one?
>
> huh, I'm puzzled. That's exactly what my patch 1/3 adds in the first place:
>
> ---8<---
> --- a/source3/modules/vfs_gpfs.c
> +++ b/source3/modules/vfs_gpfs.c
> @@ -1596,6 +1596,9 @@ static NTSTATUS vfs_gpfs_get_dos_attributes(struct vfs_handle_struct *handle,
> }
>
> *dosmode |= vfs_gpfs_winattrs_to_dosmode(attrs.winAttrs);
> + smb_fname->st.st_ex_calculated_birthtime = false;
> + smb_fname->st.st_ex_btime.tv_sec = attrs.creationTime.tv_sec;
> + smb_fname->st.st_ex_btime.tv_nsec = attrs.creationTime.tv_nsec;
>
> return NT_STATUS_OK;
> }
> @@ -1628,6 +1631,9 @@ static NTSTATUS vfs_gpfs_fget_dos_attributes(struct vfs_handle_struct *handle,
> }
>
> *dosmode |= vfs_gpfs_winattrs_to_dosmode(attrs.winAttrs);
> + fsp->fsp_name->st.st_ex_calculated_birthtime = false;
> + fsp->fsp_name->st.st_ex_btime.tv_sec = attrs.creationTime.tv_sec;
> + fsp->fsp_name->st.st_ex_btime.tv_nsec = attrs.creationTime.tv_nsec;
>
> return NT_STATUS_OK;
> }
> --
> ---8<---
>
> It's using struct smb_filename in the path based vfs_gpfs_get_dos_attributes()
> and files_struct in the handle based vfs_gpfs_fget_dos_attributes().
>
> How are you applying the patch and are you sure you're applying on top of
> master?
>
> *scratches head*
$ git am ~/vfs_gpfs_btime.patch
Applying: vfs_gpfs: update btime in vfs_gpfs_(f)get_dos_attributes
Applying: vfs_gpfs: remove updating btime from stat VFS calls
Applying: vfs_gpfs: simplify stat_with_capability() ifdef
For some reason, git am decides to switch the two parts for me. As long
as the correct version ends up in master, i do not see a problem.
>
> > > From 8902d885db3e7ce83e41cb129037e1bc1eecb0ca Mon Sep 17 00:00:00 2001
> > > From: Ralph Boehme <slow at samba.org>
> > > Date: Thu, 15 Dec 2016 18:10:22 +0100
> > > Subject: [PATCH 3/3] vfs_gpfs: simplify stat_with_capability() ifdef
> > >
> > > Signed-off-by: Ralph Boehme <slow at samba.org>
> > > ---
> > > source3/modules/vfs_gpfs.c | 10 ++++------
> > > 1 file changed, 4 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/source3/modules/vfs_gpfs.c b/source3/modules/vfs_gpfs.c
> > > index fedb5dd..768bf63 100644
> > > --- a/source3/modules/vfs_gpfs.c
> > > +++ b/source3/modules/vfs_gpfs.c
> > > @@ -1706,10 +1706,10 @@ static NTSTATUS vfs_gpfs_fset_dos_attributes(struct vfs_handle_struct *handle,
> > > return NT_STATUS_OK;
> > > }
> > >
> > > -#if defined(HAVE_FSTATAT)
> > > static int stat_with_capability(struct vfs_handle_struct *handle,
> > > struct smb_filename *smb_fname, int flag)
> > > {
> > > +#if defined(HAVE_FSTATAT)
> > > int fd = -1;
> > > bool b;
> > > char *dir_name;
> > > @@ -1743,8 +1743,10 @@ static int stat_with_capability(struct vfs_handle_struct *handle,
> > > }
> > >
> > > return ret;
> > > -}
> > > +#else
> > > + return -1;
> > > #endif
> > > +}
> >
> > My preference here would be avoiding ifdefs in the middle of the code and
> > change that to:
> >
> > #if defined(HAVE_FSTATAT)
> > static int stat_with_capability(struct vfs_handle_struct *handle,
> > struct smb_filename *smb_fname, int flag)
> > {
> > ...
> > }
> > #else
> > static int stat_with_capability(struct vfs_handle_struct *handle,
> > struct smb_filename *smb_fname, int flag)
> > {
> > return -1;
> > }
> > #endif
> >
>
> The code bases uses both forms, but imo shorter wins. :) Cf vfs_default.c, it
> uses the shorter form as well. But this commit is just a cleanup, so we can just
> drop it as well.
The part of removing the ifdefs from the vfs_gpfs_*stat calls is good.
The other is just a personal preference, so feel free to push either
version:
Reviewed-by: Christof Schmitt <cs at samba.org>
Christof
More information about the samba-technical
mailing list