* [bug report] btrfs: subpage: only call btrfs_alloc_subpage() when sectorsize is smaller than PAGE_SIZE
@ 2021-08-25 10:21 Dan Carpenter
2021-08-25 10:43 ` Qu Wenruo
0 siblings, 1 reply; 2+ messages in thread
From: Dan Carpenter @ 2021-08-25 10:21 UTC (permalink / raw)
To: wqu; +Cc: linux-btrfs
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
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);
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.
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.
6174 ASSERT(!ret);
6175 /*
6176 * To inform we have extra eb under allocation, so that
regards,
dan carpenter
^ permalink raw reply [flat|nested] 2+ messages in thread
* Re: [bug report] btrfs: subpage: only call btrfs_alloc_subpage() when sectorsize is smaller than PAGE_SIZE
2021-08-25 10:21 [bug report] btrfs: subpage: only call btrfs_alloc_subpage() when sectorsize is smaller than PAGE_SIZE Dan Carpenter
@ 2021-08-25 10:43 ` Qu Wenruo
0 siblings, 0 replies; 2+ messages in thread
From: Qu Wenruo @ 2021-08-25 10:43 UTC (permalink / raw)
To: Dan Carpenter, wqu; +Cc: linux-btrfs
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
>
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2021-08-25 10:43 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-08-25 10:21 [bug report] btrfs: subpage: only call btrfs_alloc_subpage() when sectorsize is smaller than PAGE_SIZE Dan Carpenter
2021-08-25 10:43 ` Qu Wenruo
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).