Re: [PATCH] Bug 3150: do not start useless unlinkd.

Hi Amos.

On Fri, 07 Oct 2011 20:06:19 +1300, Amos Jeffries <squid3_at_treenet.co.nz> wrote:
> On 07/10/11 18:53, Dmitry Kurochkin wrote:
> >
> > Unlinkd is used only by UFS storage. Bug before the change, Squid
> > always started unlinkd if it is built, even if it is not needed.
> > Moreover, unlinkd was started in non-daemon mode.
> >
> > The patch adds unlinks() method for SwapDir to determine if it uses
> > unlinkd. Unlinkd is started iff:
> >
> > * Squid is running in daemon mode
> > * a configured cache_dir uses unlinkd
> >
> > After the change, unlinkdClose() may be called when unlinkd was never
> > started. The patch removes a warning which was printed in this case
> > on Windows.
> > ---
> > src/SwapDir.h | 2 ++
> > src/fs/ufs/store_dir_ufs.cc | 6 ++++++
> > src/fs/ufs/ufscommon.h | 1 +
> > src/main.cc | 3 ++-
> > src/protos.h | 1 +
> > src/unlinkd.cc | 21 +++++++++++++++++++--
> > 6 files changed, 31 insertions(+), 3 deletions(-)
> >
>
> There are a few important details overlooked in this patch.
>
> The use of unlinkd is a property of cache_dir DiskIO strategy, not the
> SwapDir type itself.

I wrongly assumed that all DiskIO strategies use unlinkd. Now I see
that is not correct (e.g. DiskThreads does not use unlinkd).

But making decision whether to run unlinkd solely based on the strategy,
is not correct. For example, Rock store may use IpcIo or Blocking
DiskIO strategy, and both may use unlinkd. But Rock does not use
unlinkFile() and does use unlinkd.

I think that unlinkd usage is a property of both DiskIO strategy and
SwapDir type.

> Definitely not the use of -N option on startup.
> The disabling of unlinkd on -N will cause problems on all systems
> needing ufs or diskd in that mode. Meaning all BSD default configs, and
> those OS using upstart (Mac, Debian, and derivatives) which utilizes -N
> mode to get direct control of a single worker process.
>

I did not know that -N was used for anything but testing and debugging.
Perhaps we should also use diskers in -N mode?

(Though, neither Mac OS X nor Debian (by default) use upstart. Ubuntu
does.)

> If unlindNeeded() intention was to see that unlinkd was always enabled
> for mmapped and IPC DiskIO methods unless daemon mode was used, then it
> should be inverted to identify the true case and placed after the for loop.
>

I did not understand this, but I hope my answer clarifies.

> But, I think the better way to cover that is to move the unlinks() T/F
> decision down to the particular DiskIO strategy and have SwapDir check
> relay the answer from there.
>

We should add mayUseUnlinkd() method to DiskIOStrategy. But only UFS
SwapDir would relay answer from it. Other store modules do not need
unlinkd even if the underlying IO strategy may use it. Do you agree?

Regards,
  Dmitry

> Amos
> --
> Please be using
> Current Stable Squid 2.7.STABLE9 or 3.1.15
> Beta testers wanted for 3.2.0.12
Received on Fri Oct 07 2011 - 07:50:59 MDT