[PATCHES] fix flapping offline flag in gpfs module

[Michael Adam]

On 2014-07-18 at 13:32 -0700, Christof Schmitt wrote:
> On Fri, Jul 18, 2014 at 11:22:28AM +0200, Volker Lendecke wrote:
> > On Fri, Jul 18, 2014 at 12:19:00AM +0200, Michael Adam wrote:
> > > Attached find (as a first step) a patcheset that removes
> > > the use of stat_ex.vfs_private from vfs_gpfs by calling
> > > SMB_VFS_IS_OFFLINE() as discussed above
> > > 
> > > I guess that this also fixes a few more errors regarding
> > > reaction to of assumed offline status in the module.
> > > 
> > > Review/push appreciated.
> > 
> > Find the patches with my R-B and two typo fixes in the commit messages
> > attached. This also contains the vfs_private removal, which is a good
> > thing from my point of view. I think vfs_private is dangerous at best,
> > there is no protection at all against conflicting use in multiple
> > modules. If we really need such a thing, I think we would have to add
> > something like the fsp extensions. But I would really like to know why
> > vfs_private is really needed.
> > 
> > When looking at this area, I'd much rather see winattrs officially added
> > to stat_ex, potentially with an additional flag that the winattrs are
> > not valid here. There's at least GPFS that has special support for the
> > 4 windows bits, and I could see carving out 4 bits from an inode to be
> > a very worthwhile optimization for specialized file systems. An xattr
> > potentially has much more overhead. And as we can then store the OFFLINE
> > flag in the winattrs, we'd have a much cleaner way to do this.
> > 
> > When adding a validity-bit to the winattrs, this would then
> > also lead to one further step: statlite, as referenced in
> > http://www.mcs.anl.gov/uploads/cels/papers/TM-302-FINAL.pdf as
> > an important performance optimization in the networked file system
> > world. It can be very difficult to correctly query the mtime and file
> > size, and in particular in unix_convert() Samba does not need that
> > information at all.  Also, if a file is heavily accessed concurrently,
> > Samba typically maintains its own write timestamp. We don't have to ask
> > the file system for the correct mtime in a SMB2_FIND.  But that's much
> > further down the road.
> 
> My main concern with patches 2 and 3 would be that this now triggers a
> notification after each read or write request. On busy systems, that
> will likely affect performance.

After each read and write request?
I don't quite see that. Only if the file is offline.

The intent was of course to not modify behaviour,
but it is in fact modified by the patches, because
the original code was operating on a cache (broken,
but a cache anyways), and the new code gets the live
data. Hence I need to modify the patch to call
was_offline = IS_OFFLINE() before e.g. calling NEXT_PREAD()
and then checking was_offline after NEXT_PREAD. etc.

Will post an updated patchset...

Cheers - Michael

> We probably need a way to cache at least the offline bit. The current
> uses of the OFFLINE flag in vfs_gpfs are only in functions that have a
> fsp, so we could add a FSP_EXTENSION as a workaround in vfs_gpfs.
> 
> I agree that a better way would be have the flags (at least OFFLINE) in
> stat_ex. When doing this, we should also add a proper interface to get
> and set the flags in the vfs modules. The current approach of vfs_gpfs
> intercepting the set_xattr and get_xattr calls seems backwards. Either
> we need explicit SET_ATTR and GET_ATTR calls, or require that all STAT
> calls update the attributes in stat_ex and also add FSTATAT.
> 
> Moving towards statlite support would be great, especially since we
> already have that in gpfs.
> 
> > > Further steps: extract the hsm-logic using is_offline()
> > > from vfs_gpfs and vfs_tsmsm to one universally useful module vfs_hsm.
> > > Will happily code this but might take a little time  until I
> > > get to really doing it.
> > 
> > +1.
> 
> +1 from me too. I can also put this on my todo list, but i am not sure
> when i will get to it.
> 
> Christof
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://lists.samba.org/pipermail/samba-technical/attachments/20140720/a251cddf/attachment.pgp>