On Fri, Feb 09 2024 at 8:06P -0500,
Dan Carpenter <[email protected]> wrote:
> Hello Matthew Sakai,
>
> The patch e08b7fe0c6fa: "dm vdo: implement the chapter volume store"
> from Nov 16, 2023 (linux-next), leads to the following Smatch static
> checker warning:
>
> drivers/md/dm-vdo/volume.c:597 process_entry() warn: inconsistent returns
> '&volume->read_threads_mutex'.
> Locked on : 552,580,588,597
> Unlocked on: 565
>
> drivers/md/dm-vdo/volume.c
> 542 static int process_entry(struct volume *volume, struct queued_read
> *entry)
> 543 {
> 544 u32 page_number = entry->physical_page;
> 545 struct uds_request *request;
> 546 struct cached_page *page = NULL;
> 547 u8 *page_data;
> 548 int result;
> 549
> 550 if (entry->invalid) {
> 551 uds_log_debug("Requeuing requests for invalid page");
> 552 return UDS_SUCCESS;
> 553 }
> 554
> 555 page = select_victim_in_cache(&volume->page_cache);
> 556
> 557 uds_unlock_mutex(&volume->read_threads_mutex);
> 558 page_data = dm_bufio_read(volume->client, page_number,
> &page->buffer);
> 559 if (IS_ERR(page_data)) {
> 560 result = -PTR_ERR(page_data);
> 561 uds_log_warning_strerror(result,
> 562 "error reading physical page
> %u from volume",
> 563 page_number);
> 564 cancel_page_in_cache(&volume->page_cache,
> page_number, page);
> 565 return result;
>
> This is the only return where we aren't holding
> uds_lock_mutex(&volume->read_threads_mutex). It looks like this
> will lead to a double unlock in the caller.
I've fixed this and added your Reported-by
Thanks,
Mike
>
> 566 }
> 567 uds_lock_mutex(&volume->read_threads_mutex);
> 568
> 569 if (entry->invalid) {
> 570 uds_log_warning("Page %u invalidated after read",
> page_number);
> 571 cancel_page_in_cache(&volume->page_cache,
> page_number, page);
> 572 return UDS_SUCCESS;
> 573 }
> 574
> 575 if (!is_record_page(volume->geometry, page_number)) {
> 576 result = initialize_index_page(volume, page_number,
> page);
> 577 if (result != UDS_SUCCESS) {
> 578 uds_log_warning("Error initializing chapter
> index page");
> 579 cancel_page_in_cache(&volume->page_cache,
> page_number, page);
> 580 return result;
> 581 }
> 582 }
> 583
> 584 result = put_page_in_cache(&volume->page_cache, page_number,
> page);
> 585 if (result != UDS_SUCCESS) {
> 586 uds_log_warning("Error putting page %u in cache",
> page_number);
> 587 cancel_page_in_cache(&volume->page_cache,
> page_number, page);
> 588 return result;
> 589 }
> 590
> 591 request = entry->first_request;
> 592 while ((request != NULL) && (result == UDS_SUCCESS)) {
> 593 result = search_page(page, volume, request,
> page_number);
> 594 request = request->next_request;
> 595 }
> 596
> --> 597 return result;
> 598 }
>
> regards,
> dan carpenter
>
