Re: [bug report] btrfs: subpage: only call btrfs_alloc_subpage() when sectorsize is smaller than PAGE_SIZE

[Qu Wenruo <quwenruo.btrfs@gmx.com>]

On 2021/8/25 下午6:21, Dan Carpenter wrote:
> Hello Qu Wenruo,
>
> The patch 4c1e934ee490: "btrfs: subpage: only call
> btrfs_alloc_subpage() when sectorsize is smaller than PAGE_SIZE" from
> Aug 17, 2021, leads to the following
> Smatch static checker warning:
>
> 	fs/btrfs/subpage.c:110 btrfs_attach_subpage()
> 	warn: sleeping in atomic context

This looks like a false alert.

>
> fs/btrfs/subpage.c
>      94 int btrfs_attach_subpage(const struct btrfs_fs_info *fs_info,
>      95                          struct page *page, enum btrfs_subpage_type type)
>      96 {
>      97         struct btrfs_subpage *subpage;
>      98
>      99         /*
>      100          * We have cases like a dummy extent buffer page, which is not mappped
>      101          * and doesn't need to be locked.
>      102          */
>      103         if (page->mapping)
>      104                 ASSERT(PageLocked(page));
>      105
>      106         /* Either not subpage, or the page already has private attached */
>      107         if (fs_info->sectorsize == PAGE_SIZE || PagePrivate(page))
>      108                 return 0;
>      109
> --> 110         subpage = btrfs_alloc_subpage(fs_info, type);

Here we only alloc when sectorsize < PAGE_SIZE and the page has no private.

>      111         if (IS_ERR(subpage))
>      112                 return  PTR_ERR(subpage);
>      113
>      114         attach_page_private(page, subpage);
>      115         return 0;
>      116 }
>
> The call tree is:
>
> alloc_extent_buffer() <- disables preempt
> -> attach_extent_buffer_page()
>     -> btrfs_attach_subpage()
>
> fs/btrfs/extent_io.c
>    6132          for (i = 0; i < num_pages; i++, index++) {
>    6133                  struct btrfs_subpage *prealloc = NULL;
>    6134
>    6135                  p = find_or_create_page(mapping, index, GFP_NOFS|__GFP_NOFAIL);
>    6136                  if (!p) {
>    6137                          exists = ERR_PTR(-ENOMEM);
>    6138                          goto free_eb;
>    6139                  }
>    6140
>    6141                  /*
>    6142                   * Preallocate page->private for subpage case, so that we won't
>    6143                   * allocate memory with private_lock hold.  The memory will be
>    6144                   * freed by attach_extent_buffer_page() or freed manually if
>    6145                   * we exit earlier.
>    6146                   *
>    6147                   * Although we have ensured one subpage eb can only have one
>    6148                   * page, but it may change in the future for 16K page size
>    6149                   * support, so we still preallocate the memory in the loop.
>    6150                   */
>    6151                  if (fs_info->sectorsize < PAGE_SIZE) {
>
> The patch adds this check which means we only preallocate it when it's
> small.
>
>    6152                          prealloc = btrfs_alloc_subpage(fs_info, BTRFS_SUBPAGE_METADATA);
>    6153                          if (IS_ERR(prealloc)) {
>    6154                                  ret = PTR_ERR(prealloc);
>    6155                                  unlock_page(p);
>    6156                                  put_page(p);
>    6157                                  exists = ERR_PTR(ret);
>    6158                                  goto free_eb;
>    6159                          }
>    6160                  }
>    6161
>    6162                  spin_lock(&mapping->private_lock);
>                          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> Take a spinlock.
>
>    6163                  exists = grab_extent_buffer(fs_info, p);
>    6164                  if (exists) {
>    6165                          spin_unlock(&mapping->private_lock);
>    6166                          unlock_page(p);
>    6167                          put_page(p);
>    6168                          mark_extent_buffer_accessed(exists, p);
>    6169                          btrfs_free_subpage(prealloc);
>    6170                          goto free_eb;
>    6171                  }
>    6172                  /* Should not fail, as we have preallocated the memory */
>                             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> This comment is out of date.

The comment is still correct.

>
>
>    6173                  ret = attach_extent_buffer_page(eb, p, prealloc);
>                                                                 ^^^^^^^^
> If we don't preallocate it, then it leads to a sleeping while holding
> a spinlock bug.

If we don't preallocate it, we won't allocate it neither inside in
btrfs_attach_subpage().

Thanks,
Qu
>
>    6174                  ASSERT(!ret);
>    6175                  /*
>    6176                   * To inform we have extra eb under allocation, so that
>
> regards,
> dan carpenter
>