Re: [PATCH v3] mm, memcg: fix error return value of mem_cgroup_css_alloc()

[Yafang Shao <laoar.shao@gmail.com>]

On Tue, Apr 7, 2020 at 2:43 PM Michal Hocko <mhocko@kernel.org> wrote:
>
> [I have only now noticed there was another version posted. Please try to
> wait a bit longer for other feedback before reposting a newer version]
>

Sure I will. sorry about that.
But after we send a patch, we always don't know in which state this
patch is, e.g. Changes Requested, Not Applicable, Rejected, Needs
Review / ACK and etc, as listed in netdev list[1]. That could give us
an explicit instruction on what to do next.

[1]. https://patchwork.ozlabs.org/project/netdev/list/?state=*&page=2&param=3

> On Tue 07-04-20 11:11:13, Yafang Shao wrote:
> > On Tue, Apr 7, 2020 at 11:09 AM Andrew Morton <akpm@linux-foundation.org> wrote:
> > >
> > > On Tue, 7 Apr 2020 11:02:31 +0800 Yafang Shao <laoar.shao@gmail.com> wrote:
> > >
> > > > On Tue, Apr 7, 2020 at 7:23 AM Andrew Morton <akpm@linux-foundation.org> wrote:
> > > > >
> > > > > On Tue,  7 Apr 2020 00:56:03 +0800 Yafang Shao <laoar.shao@gmail.com> wrote:
> > > > >
> > > > > > When I run my memcg testcase which creates lots of memcgs, I found
> > > > > > there're unexpected out of memory logs while there're still enough
> > > > > > available free memory. The error log is,
> > > > > > mkdir: cannot create directory 'foo.65533': Cannot allocate memory
> > > > > >
> > > > > > The reason is when we try to create more than MEM_CGROUP_ID_MAX memcgs, an
> > > > > > -ENOMEM errno will be set by mem_cgroup_css_alloc(), but the right errno
> > > > > > should be -EBUSY "Device or resource busy". That is same with
> > > > > > memcg_alloc_cache_id().
>
> See my comment about EBUSY in the previous version
> http://lkml.kernel.org/r/20200407063621.GA18914@dhcp22.suse.cz
>

From the perspective of mkdir(2), it is better to use ENOSPC here. But
I'm not sure whether it is better to update the man page of mkdir(2)
or not.

I checked the explaination about ENOSPC in 73f576c04b94 ("mm:
memcontrol: fix cgroup creation failure after many small jobs")
carefully, but I don't a clear idea which one is better now.

Per Matthew, EBUSY is better, while per you, ENOSPC is better.

Matthew, would you mind to give more detailed explanation ?

> > > > > > As the errno really misled me, we should make it right. After this patch,
> > > > > > the error log will be,
> > > > > > mkdir: cannot create directory 'foo.65533': Device or resource busy
> > > > >
> > > > > Thanks.
> > > > >
> > > > > Was a -stable backport considered?
> > > >
> > > > I only backported to our kernel version 4.18, but I'm not sure whether
> > > > it will apply to -stable or not.
> > > > Seems this issue is introduced long time ago. I will try to backport
> > > > it to -stable.
> > > > Should I try it based on 5.5.y and 5.6.y only ? Or all the LTS kernel
> > > > versions ?
> > >
> > > What I'm asking (of you and of reviewers) is whether this issue is
> > > sufficiently serious to require fixing in -stable kernels.  The
> > > patch merging details can be sorted out later.
> > >
> > >
> >
> > I think it is worth to cc stable, as this errno will mislead the user.
>
> We have idr failure path since 73f576c04b94 ("mm: memcontrol: fix cgroup
> creation failure after many small jobs") which is more than four years
> ago without anybody ever noticing. While I do agree that the existing
> behavior might be confusing I am not really sure this qualifies as
> stable material TBH.

OK

Thanks
Yafang