Re: [PATCH] mm/zsmalloc: don't fail if can't create debugfs info

[Dan Streetman]

On Fri, Apr 29, 2016 at 1:37 AM, Minchan Kim <minchan@xxxxxxxxxx> wrote:
> On Fri, Apr 29, 2016 at 09:38:24AM +0900, Sergey Senozhatsky wrote:
>> On (04/28/16 15:07), Andrew Morton wrote:
>> > Needed a bit of tweaking due to
>> > http://ozlabs.org/~akpm/mmotm/broken-out/zsmalloc-reordering-function-parameter.patch
>>
>> Thanks.
>>
>> > From: Dan Streetman <ddstreet@xxxxxxxx>
>> > Subject: mm/zsmalloc: don't fail if can't create debugfs info
>> >
>> > Change the return type of zs_pool_stat_create() to void, and
>> > remove the logic to abort pool creation if the stat debugfs
>> > dir/file could not be created.
>> >
>> > The debugfs stat file is for debugging/information only, and doesn't
>> > affect operation of zsmalloc; there is no reason to abort creating
>> > the pool if the stat file can't be created.  This was seen with
>> > zswap, which used the same name for all pool creations, which caused
>> > zsmalloc to fail to create a second pool for zswap if
>> > CONFIG_ZSMALLOC_STAT was enabled.
>>
>> no real objections from me. given that both zram and zswap now provide
>> unique names for zsmalloc stats dir, this patch does not fix any "real"
>> (observed) problem /* ENOMEM in debugfs_create_dir() is a different
>> case */.  so it's more of a cosmetic patch.
>>
>
> Logically, I agree with Dan that debugfs is just optional so it
> shouldn't affect the module running *but* practically, debugfs_create_dir
> failure with no memory would be rare. Rather than it, we would see
> error from same entry naming like Dan's case.
>
> If we removes such error propagation logic in case of same naming,
> how do zsmalloc user can notice that debugfs entry was not created
> although zs_creation was successful returns success?

Does it actually matter to the caller?

Since there's no way for zsmalloc to know if the stats dir/file
creation failed because of EEXIST or because of ENOMEM, there's no way
for it to let the caller know why it failed, either.  Thus all
zsmalloc can do is return a generic error, or possibly-wrong ENOMEM.
In that case what will the caller do?  Change the name and try again?
How does the caller know what name to change it to, maybe the new name
is taken too?

The point of debugfs is to provide debug; failures should be ignored,
because it's just debug.  It should never prevent actual operation of
the driver.

>
> Otherwise, future user of zsmalloc can miss it easily if they repeates
> same mistakes. So, what's the gain with this patch in real practice?

Well as far as future users, zs_create_pool doesn't document 'name' at
all, and certainly doesn't clarify that 'name' should be unique across
*all* zs pools that exist.  And zsmalloc behavior should not be
different depending on whether the ZSMALLOC_STAT param - which appears
to be a debug/info only param - is enabled or not.

But after any future driver using zsmalloc is created, if it did use
an already-existing name - either because it was not coded to use
unique names, or because of a bug that reused an existing name - which
is worse?
1) driver suddenly stops working because new zs pools can't be created?
2) statistics information isn't available for some of the pools created?

And, even though zswap is now patched to provide a unique name, why
does zswap have to bear the burden of that?  zswap doesn't care at all
about the pool name, and there's no way for users to tell which
zsmalloc pool corresponds to which zswap pool parameters.  Future
users of zsmalloc (from a code point of view, not person) probably
will also not care about the zsmalloc pool name.  And zsmalloc still
logs the failure - so anyone looking for the stats and not finding it
can easily check the logs to see the reason.

The other alternative that I mentioned before, is for zsmalloc to take
care of the problem itself.  If the debugfs dir creation fails, it
should change the name and retry; or zsmalloc can keep a list of
active pool names so it knows if a new pool's name exists already or
not.  But neither expecting the calling code to retry with a different
name, nor failing pool creation, seem like a good response when the
only failure is providing debug/stats information.


>
>
>> FWIW,
>> Reviewed-by: Sergey Senozhatsky <sergey.senozhatsky@xxxxxxxxx>
>>
>>       -ss
>
> --
> To unsubscribe, send a message with 'unsubscribe linux-mm' in
> the body to majordomo@xxxxxxxxxx  For more info on Linux MM,
> see: http://www.linux-mm.org/ .
> Don't email: <a href=mailto:"dont@xxxxxxxxx";> email@xxxxxxxxx </a>