Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Sat, 2 Jul 2022 13:44:12 -0700
From:   Yury Norov <yury.norov@gmail.com>
To:     Christophe JAILLET <christophe.jaillet@wanadoo.fr>
Cc:     agk@redhat.com, snitzer@kernel.org, dm-devel@redhat.com,
        vneethv@linux.ibm.com, oberpar@linux.ibm.com, hca@linux.ibm.com,
        gor@linux.ibm.com, agordeev@linux.ibm.com,
        borntraeger@linux.ibm.com, svens@linux.ibm.com,
        almaz.alexandrovich@paragon-software.com,
        andriy.shevchenko@linux.intel.com, linux@rasmusvillemoes.dk,
        linux-s390@vger.kernel.org, ntfs3@lists.linux.dev,
        linux-kernel@vger.kernel.org, kernel-janitors@vger.kernel.org
Subject: Re: [PATCH 0/4] Introduce bitmap_size()
Message-ID: <YsCuHLTsKGCO/jsL@yury-laptop>
References: <cover.1656785856.git.christophe.jaillet@wanadoo.fr>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <cover.1656785856.git.christophe.jaillet@wanadoo.fr>
Precedence: bulk

On Sat, Jul 02, 2022 at 08:28:53PM +0200, Christophe JAILLET wrote:
> This serie introduces bitmap_size() which returns the size, in bytes, of a
> bitmap. Such a function is useful to simplify some drivers that use vmalloc() or
> other functions to allocate some butmaps.

This generally looks like a step in a wrong direction. Bitmap is by
definition an array of bits. If someone has a reason to handle bitmap
on a per-byte basis, the guy is probably doing something wrong.

We already have quite comprehensive list of functions that help to
allocate, fill, clear, copy etc bitmap without considering it as an
array of bytes or words.

> ... some drivers that use vmalloc() ...

If a driver needs vmalloc() for a bitmap, we should introduce
bitmap_vmalloc(), not bitmap_size().

> It also hides some implementation details about how bitmaps are stored (array of
> longs)

Sorry, I don't understand that. How bitmap_size() helps to hide a fact that
bitmap is an array of unsigned longs? (Except that it makes an impression
that it's an array of bytes.)

> Before introducing this function in patch 3, patch 1 and 2 rename some functions
> with the same name but with different meaning.
> 
> Finaly, patch 4 makes use of the new function in bitmap.h.

You mentioned, you need bitmap_size() to use with vmalloc(), but in
patch 4, there's no such code. 
 
> Other follow-up patches to simplify some drivers will be proposed later if/when
> this serie is merged.

This series doesn't show an example of how you're going to use new
API, and therefore it's hard to judge, do we really need bitmap_size(),
or we just need more helpers around.

As I already said, bitmaps are evolving in 2nd direction, which is the
right approach, I think.

Thanks,
Yury

> Christophe JAILLET (4):
>   s390/cio: Rename bitmap_size() as idset_bitmap_size()
>   fs/ntfs3: Rename bitmap_size() as ntfs3_bitmap_size()
>   bitmap: Introduce bitmap_size()
>   bitmap: Use bitmap_size()
> 
>  drivers/md/dm-clone-metadata.c |  5 -----
>  drivers/s390/cio/idset.c       |  8 ++++----
>  fs/ntfs3/bitmap.c              |  4 ++--
>  fs/ntfs3/fsntfs.c              |  2 +-
>  fs/ntfs3/index.c               |  6 +++---
>  fs/ntfs3/ntfs_fs.h             |  2 +-
>  fs/ntfs3/super.c               |  2 +-
>  include/linux/bitmap.h         | 15 +++++++++------
>  lib/math/prime_numbers.c       |  2 --
>  9 files changed, 21 insertions(+), 25 deletions(-)
> 
> -- 
> 2.34.1