Re: [PATCH 2/3] z3fold: don't fail kernel build if z3fold_header is too big

[Dan Streetman]

On Fri, Nov 25, 2016 at 11:25 AM, Vitaly Wool <vitalywool@xxxxxxxxx> wrote:
> On Fri, Nov 25, 2016 at 4:59 PM, Dan Streetman <ddstreet@xxxxxxxx> wrote:
>> On Tue, Nov 15, 2016 at 11:00 AM, Vitaly Wool <vitalywool@xxxxxxxxx> wrote:
>>> Currently the whole kernel build will be stopped if the size of
>>> struct z3fold_header is greater than the size of one chunk, which
>>> is 64 bytes by default. This may stand in the way of automated
>>> test/debug builds so let's remove that and just fail the z3fold
>>> initialization in such case instead.
>>>
>>> Signed-off-by: Vitaly Wool <vitalywool@xxxxxxxxx>
>>> ---
>>>  mm/z3fold.c | 11 ++++++++---
>>>  1 file changed, 8 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/mm/z3fold.c b/mm/z3fold.c
>>> index 7ad70fa..ffd9353 100644
>>> --- a/mm/z3fold.c
>>> +++ b/mm/z3fold.c
>>> @@ -870,10 +870,15 @@ MODULE_ALIAS("zpool-z3fold");
>>>
>>>  static int __init init_z3fold(void)
>>>  {
>>> -       /* Make sure the z3fold header will fit in one chunk */
>>> -       BUILD_BUG_ON(sizeof(struct z3fold_header) > ZHDR_SIZE_ALIGNED);
>>
>> Nak.  this is the wrong way to handle this.  The build bug is there to
>> indicate to you that your patch makes the header too large, not as a
>> runtime check to disable everything.
>
> Okay, let's agree to drop it.
>
>> The right way to handle it is to change the hardcoded assumption that
>> the header fits into a single chunk; e.g.:
>>
>> #define ZHDR_SIZE_ALIGNED round_up(sizeof(struct z3fold_header), CHUNK_SIZE)
>> #define ZHDR_CHUNKS (ZHDR_SIZE_ALIGNED >> CHUNK_SHIFT)
>>
>> then use ZHDR_CHUNKS in all places where it's currently assumed the
>> header is 1 chunk, e.g. in num_free_chunks:
>>
>>   if (zhdr->middle_chunks != 0) {
>>     int nfree_before = zhdr->first_chunks ?
>> -      0 : zhdr->start_middle - 1;
>> +      0 : zhdr->start_middle - ZHDR_CHUNKS;
>>
>> after changing all needed places like that, the build bug isn't needed
>> anymore (unless we want to make sure the header isn't larger than some
>> arbitrary number N chunks)
>
> That sounds overly complicated to me. I would rather use bit_spin_lock
> as Arnd suggested. What would you say?

using the correctly-calculated header size instead of a hardcoded
value is overly complicated?  i don't agree with that...i'd say it
should have been done in the first place ;-)

bit spin locks are hard to debug and slower and should only be used
where space really is absolutely required to be minimal, which
definitely isn't the case here.  this should use regular spin locks
and change the hardcoded assumption of zhdr size < chunk size (which
always was a bad assumption) to calculate it correctly.  it's really
not that hard; there are only a few places where the offset position
of the chunks is calculated.


>
> ~vitaly