* [bug report] dm vdo: implement the chapter volume store
@ 2024-02-09 13:06 Dan Carpenter
2024-02-11 20:00 ` Mike Snitzer
0 siblings, 1 reply; 2+ messages in thread
From: Dan Carpenter @ 2024-02-09 13:06 UTC (permalink / raw)
To: msakai; +Cc: dm-devel
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.
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
^ permalink raw reply [flat|nested] 2+ messages in thread
* Re: [bug report] dm vdo: implement the chapter volume store
2024-02-09 13:06 [bug report] dm vdo: implement the chapter volume store Dan Carpenter
@ 2024-02-11 20:00 ` Mike Snitzer
0 siblings, 0 replies; 2+ messages in thread
From: Mike Snitzer @ 2024-02-11 20:00 UTC (permalink / raw)
To: Dan Carpenter; +Cc: msakai, dm-devel
On Fri, Feb 09 2024 at 8:06P -0500,
Dan Carpenter <dan.carpenter@linaro.org> 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
>
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2024-02-11 20:00 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-02-09 13:06 [bug report] dm vdo: implement the chapter volume store Dan Carpenter
2024-02-11 20:00 ` Mike Snitzer
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.