Remove serverids_exist from parse_share_modes?
ira at samba.org
Tue May 15 17:34:07 MDT 2012
On Tue, May 15, 2012 at 5:56 PM, Andrew Bartlett <abartlet at samba.org> wrote:
> On Tue, 2012-05-15 at 16:41 +0200, Volker Lendecke wrote:
> > Hi!
> > Under
> > https://git.samba.org/?p=vl/samba.git/.git;a=shortlog;h=refs/heads/wip
> > find a patchset that removes the serverids_exist check from
> > parse_share_modes. It replaces the central check by checks
> > when conflicts happen. Thus it removes the assumption that
> > the data from get_share_mode_lock & friends only contains
> > entries with live processes attached. In a normally
> > functioning server, this is certainly still the case because
> > everybody is supposed to clean up behind itself. The
> > different behavior shows when smbds crash hard.
> > There are two reasons for this change. For busy files (for
> > example a popular directory everybody watches with notify)
> > we are beating the serverid.tdb pretty hard by checking
> > every entry on every open and close possibly multiple times.
> > This is a penalty for well-functioning servers. In this
> > particular case those checks are not strictly required
> > anyway because no oplocks are granted that would need to be
> > revoked and no share mode locks will happen. So we could
> > live with stale entries. The second reason for this change
> > is a requirement from the implementation of durable file
> > handles. With DH we can not clean up all entries without
> > processes behind, this needs to be more fine-grained.
> > I have tried to catch all cases where we rely on proper
> > cleanup. The cleanup3 test plays some tricks with
> > locking.tdb, forcing smbd to excercise some of the new code.
> > Unfortunately we can't run this in make test because we have
> > no access to the server's locking.tdb from smbtorture, so we
> > have to rely on manual tests for this new functionality.
> Why can't we access locking.tdb from smbtorture in make test? We have
> plenty of other tests that access the server-side files in the share, so
> accessing locking.tdb shouldn't be too hard.
> > There's one downside so far: For crashing servers the code
> > leaves stale entries behind as long as they do not conflict
> > with other opens. I am not sure it is worth a regular
> > cleanup process which would put quite some load on the tdb
> > files whenever it is run on large and busy servers.
> > Comments?
> How often do servers under heavy load crash?
Often enough that I don't have a server w/o crashdumps.
But in reality the issue is not that bad. If there IS a conflict, it
is looked up and cleaned up. Otherwise, there's a small amount of
extra records that take longer to clean up. I don't see this as a
show-stopper. (And if someone was going to, trust me, I'd be way up
on the list.)
s3: Add "share_mode_stale_server" -
Please include the real name of the function in the commit, it makes
the rest of the reading hang together better.
-- General --
Does get_delete_on_close_token and defer_open need the same treatment?
-- Closing comments --
I hope this helped out, and allows Jeremy to find more fun things ;).
More information about the samba-technical