[PATCHES] fix flapping offline flag in gpfs module

[Christof Schmitt]

On Fri, Jul 11, 2014 at 12:22:35AM +0200, Michael Adam wrote:
> Hi Christof,
> 
> On 2014-07-10 at 10:46 -0700, Christof Schmitt wrote:
> > On Thu, Jul 10, 2014 at 10:54:30AM +0200, Volker Lendecke wrote:
> > > On Thu, Jul 10, 2014 at 12:38:43AM +0200, Michael Adam wrote:
> > > > There is this awful problem of the flapping offline flag
> > > > when using the gpfs module and "gpfs:winattr = yes".
> > > > It was pretty clear since a time that this must be due
> > > > to uninitialized data (where the winattrs are stored),
> > > > i.e. the vfs_private in the struct stat_ex.
> > > 
> > > As discussed offline with Michael: I think with patch 2/4
> > > vfs_private is not referenced at all anymore. vfs_private is
> > > a broken concept once more than one VFS module starts using
> > > it. IMHO it should go.
> > 
> > Other parts of the vfs_gpfs module still access the vfs_private.
> 
> Right, but I think that most of these accesses are potentially
> invalid, except for the ones setting it: They all check for
> (or reset) the offline flag. Hence they all may suffer from
> the same problem as the one in is_offline: They may not operate
> on properly intialized data. Some don't even check for
> VALID_STAT().
> 
> They should probably use SMB_VFS_IS_OFFLINE() (and
> SMB_VFS_SET_OFFLINE()).
> And then vfs_private could easily be removed.

Yes. The question is if this affects performance, but it is certainly
better to be correct first and then optimize later.

> Thinking about it: these uses of the offline check
> are more or less exactly the same as in the tsmsm module.
> (Methods: aio_force, sendfile, pread, preade_recv, pwrite,
> pwrite_recv, only the open meth is additional in gpfs).

Even the check in open is not tied to GPFS. It is just meant to prevent
mass-recalls of files from clients that do not check the offline flag.
If we move the checks to vfs_hsm, then we should move this one, too.

> All this is generic treatment of a hsm system.
> There seems to be nothing special in this, nor in
> tsmsm. So, wouldn't it be a good improvement to
> extract this logic from vfs_gpfs and vfs_tsmsm
> to a new module, say vfs_hsm. Then only the detection
> (and setting) of offline files would remain specific
> to vfs_gpfs and others.
> 
> Does this sound like a plan?

Yes, there is certainly duplicated code that should be avoided. I assume
that the new module would call SMB_VFS_IS_OFFLINE and that call is
then intercepted by vfs_gpfs or vfs_tsmsm.

Are you going to write a patch? :-)

> > I agree that it should go, we just have to replace it with a FSP_EXTENSION.
> 
> Yeah maybe in a next step, but not required for removal of
> vfs_private, imho.

Right, if we just call IS_OFFLINE, then we don't need to cache the
flags.

Christof