> If there is no other way to avoid the Smatch warning,
The Smatch warning is not the issue. If we're holding a spinlock and
we call might_sleep() then that generates a stack trace at runtime if
you have CONFIG_DEBUG_ATOMIC_SLEEP enabled. Probably enabling
CONFIG_DEBUG_ATOMIC_SLEEP should just be a standard part of the QC
process.
Anyway, it sounds you're just doing the locking to silence a warning in
xarray. Let's ask Matthew if he has a hint.
regards,
dan carpenter
On Sun, May 08, 2022 at 08:03:14PM +0200, Bodo Stroesser wrote:
> Hi Dan,
>
> Thank you for the report.
>
> I'm quite sure that our code does not cause any problems, because
> in tcmu we explicitly or implicitly take the xarray's lock only while
> holding the so called cmdr_lock mutex. Also, we take the lock without
> disabling irq or bh. So I think there is no problem if lock_page sleeps
> while we hold the xarray's lock.
>
> Of course, we wouldn't need the xarray lock at all, but xarray code
> forces us to take it to avoid warnings.
>
> In tcmu_blocks_release we use the advanced xarray API to keep the
> overhead small. It allows us to lock/unlock before and after the loop
> only. If there is no other way to avoid the Smatch warning, we could
> easily put additional xas_unlock() and xas_lock() around the
> lock_page/unlock_page block.
>
> But if there is a way to avoid the warning without imposing overhead,
> I would of course prefer it.
>
> Regards,
> Bodo
>
>
> On 04.05.22 17:12, Dan Carpenter wrote:
> > Hello Xiaoguang Wang,
> >
> > The patch bb9b9eb0ae2e: "scsi: target: tcmu: Fix possible data
> > corruption" from Apr 21, 2022, leads to the following Smatch static
> > checker warning:
> >
> > drivers/target/target_core_user.c:1689 tcmu_blocks_release()
> > warn: sleeping in atomic context
> >
> > drivers/target/target_core_user.c
> > 1661 static u32 tcmu_blocks_release(struct tcmu_dev *udev, unsigned long first,
> > 1662 unsigned long last)
> > 1663 {
> > 1664 XA_STATE(xas, &udev->data_pages, first * udev->data_pages_per_blk);
> > 1665 struct page *page;
> > 1666 u32 pages_freed = 0;
> > 1667
> > 1668 xas_lock(&xas);
> > ^^^^^^^^^^^^^^
> > We take a spinlock here.
> >
> >
> > 1669 xas_for_each(&xas, page, (last + 1) * udev->data_pages_per_blk - 1) {
> > 1670 xas_store(&xas, NULL);
> > 1671 /*
> > 1672 * While reaching here there may be page faults occurring on
> > 1673 * the to-be-released pages. A race condition may occur if
> > 1674 * unmap_mapping_range() is called before page faults on these
> > 1675 * pages have completed; a valid but stale map is created.
> > 1676 *
> > 1677 * If another command subsequently runs and needs to extend
> > 1678 * dbi_thresh, it may reuse the slot corresponding to the
> > 1679 * previous page in data_bitmap. Though we will allocate a new
> > 1680 * page for the slot in data_area, no page fault will happen
> > 1681 * because we have a valid map. Therefore the command's data
> > 1682 * will be lost.
> > 1683 *
> > 1684 * We lock and unlock pages that are to be released to ensure
> > 1685 * all page faults have completed. This way
> > 1686 * unmap_mapping_range() can ensure stale maps are cleanly
> > 1687 * removed.
> > 1688 */
> > --> 1689 lock_page(page);
> > ^^^^^^^^^^^^^^^
> > The lock_page() function calls might_sleep() (inside the declaration
> > block).
> >
> > 1690 unlock_page(page);
> > 1691 __free_page(page);
> > 1692 pages_freed++;
> > 1693 }
> > 1694 xas_unlock(&xas);
> > ^^^^^^^^^^^^^^^^^
> > Unlock
> >
> > 1695
> > 1696 atomic_sub(pages_freed, &global_page_count);
> > 1697
> > 1698 return pages_freed;
> > 1699 }
> >
> > regards,
> > dan carpenter
