Re: [bug report] dm vdo: implement the chapter volume store

[Mike Snitzer <snitzer@kernel.org>]

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
>