On Mon, May 25, 2020 at 3:37 PM Chengguang Xu <cgxu519@xxxxxxxxxxxx> wrote:
>
> 在 5/20/2020 10:44 PM, Miklos Szeredi 写道:
> > On Tue, May 19, 2020 at 11:24 AM cgxu <cgxu519@xxxxxxxxxxxx> wrote:
> >> On 5/19/20 4:21 PM, Miklos Szeredi wrote:
> >>> On Tue, May 19, 2020 at 7:02 AM cgxu <cgxu519@xxxxxxxxxxxx> wrote:
> >>>
> >>>> If we don't consider that only drop negative dentry of our lookup,
> >>>> it is possible to do like below, isn't it?
> >>> Yes, the code looks good, though I'd consider using d_lock on dentry
> >>> instead if i_lock on parent, something like this:
> >>>
> >>> if (d_is_negative(dentry) && dentry->d_lockref.count == 1) {
> >>> spin_lock(&dentry->d_lock);
> >>> /* Recheck condition under lock */
> >>> if (d_is_negative(dentry) && dentry->d_lockref.count == 1)
> >>> __d_drop(dentry)
> >>> spin_unlock(&dentry->d_lock);
> >> And after this we will still treat 'dentry' as negative dentry and dput it
> >> regardless of the second check result of d_is_negative(dentry), right?
> > I'd restructure it in the same way as lookup_positive_unlocked()...
> >
> >>> }
> >>>
> >>> But as Amir noted, we do need to take into account the case where
> >>> lower layers are shared by multiple overlays, in which case dropping
> >>> the negative dentries could result in a performance regression.
> >>> Have you looked at that case, and the effect of this patch on negative
> >>> dentry lookup performance?
> >> The container which is affected by this feature is just take advantage
> >> of previous another container but we could not guarantee that always
> >> happening. I think there no way for best of both worlds, consider that
> >> some malicious containers continuously make negative dentries by
> >> searching non-exist files, so that page cache of clean data, clean
> >> inodes/dentries will be freed by memory reclaim. All of those
> >> behaviors will impact the performance of other container instances.
> >>
> >> On the other hand, if this feature significantly affects particular
> >> container,
> >> doesn't that mean the container is noisy neighbor and should be restricted
> >> in some way?
> > Not necessarily. Negative dentries can be useful and in case of
> > layers shared between two containers having negative dentries cached
> > in the lower layer can in theory positively affect performance. I
> > don't have data to back this up, nor the opposite. You should run
> > some numbers for container startup times with and without this patch.
>
> I did some simple tests for it but the result seems not very steady, so
> I need to take time to do more detail tests later. Is it possible to
> apply the patch for upper layer first?
Sure, that's a good start.
Thanks,
Miklos
