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

On 07/10/11 20:50, Dmitry Kurochkin wrote:
> 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.

Yes. It is up to the SwapDir to pass out the Strategies answer if it
matters (or not).

>
>> 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?

I think so. It will be on production machines, so the speed gain there
would be worth it.

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

MacOSX uses some other weird thing launchd IIRC. It's identical to
upstart in behaviour and requirements. Debian is migrating towards
upstart with admin popularity.

>
>> 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?

I agree. But please add a comment in the rock test function to state why
its not calling its Strategy for details.
  Something like: "rock storage does not have files to erase/unlink"

Amos

-- 
Please be using
   Current Stable Squid 2.7.STABLE9 or 3.1.15
   Beta testers wanted for 3.2.0.12