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