Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:7 as permitted sender) client-ip=2620:137:e000::3:7;
Date:   Mon, 18 Sep 2023 07:59:03 -0700
From:   Yury Norov <yury.norov@gmail.com>
To:     Mirsad Todorovac <mirsad.todorovac@alu.unizg.hr>
Cc:     Jan Kara <jack@suse.cz>, Philipp Stanner <pstanner@redhat.com>,
        linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org,
        Matthew Wilcox <willy@infradead.org>, Chris Mason <clm@fb.com>,
        Andrew Morton <akpm@linux-foundation.org>,
        Josef Bacik <josef@toxicpanda.com>,
        David Sterba <dsterba@suse.com>, linux-btrfs@vger.kernel.org,
        linux-mm@kvack.org
Subject: Re: [PATCH v1 1/1] xarray: fix the data-race in xas_find_chunk() by
 using READ_ONCE()
Message-ID: <ZQhlt/EbRf3Y+0jT@yury-ThinkPad>
References: <20230918044739.29782-1-mirsad.todorovac@alu.unizg.hr>
 <20230918094116.2mgquyxhnxcawxfu@quack3>
 <22ca3ad4-42ef-43bc-51d0-78aaf274977b@alu.unizg.hr>
 <20230918113840.h3mmnuyer44e5bc5@quack3>
 <fb0f5ba9-7fe3-a951-0587-640e7672efec@alu.unizg.hr>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <fb0f5ba9-7fe3-a951-0587-640e7672efec@alu.unizg.hr>
Precedence: bulk

On Mon, Sep 18, 2023 at 02:46:02PM +0200, Mirsad Todorovac wrote:

...

> Ah, I see. This is definitely not good. But I managed to fix and test the find_next_bit()
> family, but this seems that simply
> 
> -------------------------------------------
>  include/linux/xarray.h | 8 --------
>  1 file changed, 8 deletions(-)
> 
> diff --git a/include/linux/xarray.h b/include/linux/xarray.h
> index 1715fd322d62..89918b65b00d 100644
> --- a/include/linux/xarray.h
> +++ b/include/linux/xarray.h
> @@ -1718,14 +1718,6 @@ static inline unsigned int xas_find_chunk(struct xa_state *xas, bool advance,
>         if (advance)
>                 offset++;
> -       if (XA_CHUNK_SIZE == BITS_PER_LONG) {
> -               if (offset < XA_CHUNK_SIZE) {
> -                       unsigned long data = READ_ONCE(*addr) & (~0UL << offset);
> -                       if (data)
> -                               return __ffs(data);
> -               }
> -               return XA_CHUNK_SIZE;
> -       }
>         return find_next_bit(addr, XA_CHUNK_SIZE, offset);
>  }

This looks correct. As per my understanding, the removed part is the
1-word bitmap optimization for find_next_bit. If so, it's not needed
because find_next_bit() bears this optimization itself.

...

> --------------------------------------------------------
>  lib/find_bit.c | 33 +++++++++++++++++----------------
>  1 file changed, 17 insertions(+), 16 deletions(-)
> 
> diff --git a/lib/find_bit.c b/lib/find_bit.c
> index 32f99e9a670e..56244e4f744e 100644
> --- a/lib/find_bit.c
> +++ b/lib/find_bit.c
> @@ -18,6 +18,7 @@
>  #include <linux/math.h>
>  #include <linux/minmax.h>
>  #include <linux/swab.h>
> +#include <asm/rwonce.h>
>  /*
>   * Common helper for find_bit() function family
> @@ -98,7 +99,7 @@ out:                                                                          \
>   */
>  unsigned long _find_first_bit(const unsigned long *addr, unsigned long size)
>  {
> -       return FIND_FIRST_BIT(addr[idx], /* nop */, size);
> +       return FIND_FIRST_BIT(READ_ONCE(addr[idx]), /* nop */, size);
>  }
>  EXPORT_SYMBOL(_find_first_bit);
>  #endif

...

That doesn't look correct. READ_ONCE() implies that there's another
thread modifying the bitmap concurrently. This is not the true for
vast majority of bitmap API users, and I expect that forcing
READ_ONCE() would affect performance for them.

Bitmap functions, with a few rare exceptions like set_bit(), are not
thread-safe and require users to perform locking/synchronization where
needed.

If you really need READ_ONCE, I think it's better to implement a new
flavor of the function(s) separately, like:
        find_first_bit_read_once()

Thanks,
Yury