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

On 10/07/2011 03:56 AM, Amos Jeffries wrote:
> On 07/10/11 20:50, Dmitry Kurochkin wrote:
>> On Fri, 07 Oct 2011 20:06:19 +1300, Amos Jeffries 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).

Agreed. Let's assume that frequent unlinking is the only valid reason to
use unlinkd and that frequent unlinking can only be caused by store
entry removal. All SwapDirs must implement the unlink(entry) method. A
SwapDir implementation has two correct choices:

1) Handle entry removal without removing any files (Rock, COSS).
2) Remove some file using the configured DiskIO module (ufs).

For SwapDirs using option #1, the implementation of unlinks() method is
clear: It should return false unconditionally.

For SwapDirs using option #2, the answer depends on the DiskIO module in
use. SwapDir code should ask the module if it can benefit from a running
unlinkd and relay that answer to the caller.

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

-N is used for many things (unfortunately). Let's focus on unlinkd for
now. We can revisit SMP and -N relationship separately. Since -N is
often used as the preferred startup method in many production
environments that use unlinkd, we should not disable unlinkd just
because -N is on.

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

Same for COSS.

I would rename things a little so that the flow is consistent with the
above corrections. To put it all together:

::unlinkdNeeded() calls sd->unlinkdUseful() which may call
io->unlinkdUseful().

Thank you,

Alex.
Received on Fri Oct 07 2011 - 19:51:35 MDT