[PATCHES] fix flapping offline flag in gpfs module

Christof Schmitt cs at samba.org
Tue Jul 22 16:18:53 MDT 2014


On Sun, Jul 20, 2014 at 12:23:31PM +0200, Michael Adam wrote:
> 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.

Sorry i got that wrong. With the proposed patches, no notification would
get sent after a read or write. The check would be done after completing
the read or write and at that point the file is no longer flagged as
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.

Yes, check the offline flag first. If the read or write succeeded and
the file was offline, then the offline status must have changed and
other processes should get notified.

> Will post an updated patchset...

Thanks.

Christof


More information about the samba-technical mailing list