[PATCH] vfs_gpfs: move btime stuff from stat to get_dos_attrs

[Ralph Böhme]

On Thu, Dec 15, 2016 at 12:40:22PM -0700, Christof Schmitt wrote:
> 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.

This sounds really strage, I'll dig into this tomorrow, afaict soemthing like
this should not happen. *scratches head really hard*

> 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>

Cool, thanks! :)

...not pushing yet because of the issue with the fist patch...

Cheerio!
-slow