All of lore.kernel.org
 help / color / mirror / Atom feed
* [bug report] btrfs: introduce read_extent_buffer_subpage()
@ 2021-01-28 10:50 Dan Carpenter
  2021-01-28 11:06 ` Qu Wenruo
  0 siblings, 1 reply; 3+ messages in thread
From: Dan Carpenter @ 2021-01-28 10:50 UTC (permalink / raw)
  To: wqu; +Cc: linux-btrfs

Hello Qu Wenruo,

The patch 5c60a522f1ea: "btrfs: introduce
read_extent_buffer_subpage()" from Jan 16, 2021, leads to the
following static checker warning:

	fs/btrfs/extent_io.c:5797 read_extent_buffer_subpage()
	info: return a literal instead of 'ret'

fs/btrfs/extent_io.c
  5780  static int read_extent_buffer_subpage(struct extent_buffer *eb, int wait,
  5781                                        int mirror_num)
  5782  {
  5783          struct btrfs_fs_info *fs_info = eb->fs_info;
  5784          struct extent_io_tree *io_tree;
  5785          struct page *page = eb->pages[0];
  5786          struct bio *bio = NULL;
  5787          int ret = 0;
  5788  
  5789          ASSERT(!test_bit(EXTENT_BUFFER_UNMAPPED, &eb->bflags));
  5790          ASSERT(PagePrivate(page));
  5791          io_tree = &BTRFS_I(fs_info->btree_inode)->io_tree;
  5792  
  5793          if (wait == WAIT_NONE) {
  5794                  ret = try_lock_extent(io_tree, eb->start,
  5795                                        eb->start + eb->len - 1);
  5796                  if (ret <= 0)
  5797                          return ret;

If try_lock_extent() fails to get the lock and returns 0, then is
returning zero here really the correct behavior?  It feels like there
should be some documentation because this behavior is unexpected.

  5798          } else {
  5799                  ret = lock_extent(io_tree, eb->start, eb->start + eb->len - 1);
  5800                  if (ret < 0)
  5801                          return ret;
  5802          }
  5803  
  5804          ret = 0;
  5805          if (test_bit(EXTENT_BUFFER_UPTODATE, &eb->bflags) ||
  5806              PageUptodate(page) ||
  5807              btrfs_subpage_test_uptodate(fs_info, page, eb->start, eb->len)) {
  5808                  set_bit(EXTENT_BUFFER_UPTODATE, &eb->bflags);
  5809                  unlock_extent(io_tree, eb->start, eb->start + eb->len - 1);
  5810                  return ret;
  5811          }

regards,
dan carpenter

^ permalink raw reply	[flat|nested] 3+ messages in thread
* Re: [bug report] btrfs: introduce read_extent_buffer_subpage()
  2021-01-28 10:50 [bug report] btrfs: introduce read_extent_buffer_subpage() Dan Carpenter
@ 2021-01-28 11:06 ` Qu Wenruo
  2021-01-28 11:10   ` Qu Wenruo
  0 siblings, 1 reply; 3+ messages in thread
From: Qu Wenruo @ 2021-01-28 11:06 UTC (permalink / raw)
  To: Dan Carpenter, wqu; +Cc: linux-btrfs



On 2021/1/28 下午6:50, Dan Carpenter wrote:
> Hello Qu Wenruo,
>
> The patch 5c60a522f1ea: "btrfs: introduce
> read_extent_buffer_subpage()" from Jan 16, 2021, leads to the
> following static checker warning:
>
> 	fs/btrfs/extent_io.c:5797 read_extent_buffer_subpage()
> 	info: return a literal instead of 'ret'
>
> fs/btrfs/extent_io.c
>    5780  static int read_extent_buffer_subpage(struct extent_buffer *eb, int wait,
>    5781                                        int mirror_num)
>    5782  {
>    5783          struct btrfs_fs_info *fs_info = eb->fs_info;
>    5784          struct extent_io_tree *io_tree;
>    5785          struct page *page = eb->pages[0];
>    5786          struct bio *bio = NULL;
>    5787          int ret = 0;
>    5788
>    5789          ASSERT(!test_bit(EXTENT_BUFFER_UNMAPPED, &eb->bflags));
>    5790          ASSERT(PagePrivate(page));
>    5791          io_tree = &BTRFS_I(fs_info->btree_inode)->io_tree;
>    5792
>    5793          if (wait == WAIT_NONE) {
>    5794                  ret = try_lock_extent(io_tree, eb->start,
>    5795                                        eb->start + eb->len - 1);
>    5796                  if (ret <= 0)
>    5797                          return ret;
>
> If try_lock_extent() fails to get the lock and returns 0, then is
> returning zero here really the correct behavior?

This is the same behavior of read_extent_buffer_pages() for regular
sector size:

int read_extent_buffer_pages(struct extent_buffer *eb, int wait, int
mirror_num)
{
	...
         int ret = 0;
	...
	        num_pages = num_extent_pages(eb);
         for (i = 0; i < num_pages; i++) {
                 page = eb->pages[i];
                 if (wait == WAIT_NONE) {
                         if (!trylock_page(page))
                                 goto unlock_exit; <<<<
	...
unlock_exit:
         while (locked_pages > 0) {
                 locked_pages--;
                 page = eb->pages[locked_pages];
                 unlock_page(page);
         }
         return ret;
}

Here when we hit trylock_page() == false case, we directly go
unlock_exit, and by that time, @ret is still 0.


I'm not yet confident enough to say why it's OK, but my initial guess
is, we won't have (wait == WAIT_NONE) case for metadata read.

Thank you for the hint, I'll take more time to make sure the original
behavior is correct, and if it's really (wait == WAIT_NONE) will never
be true for metadata, I'll send out cleanup for this.

Thanks,
Qu

>  It feels like there
> should be some documentation because this behavior is unexpected.
>
>    5798          } else {
>    5799                  ret = lock_extent(io_tree, eb->start, eb->start + eb->len - 1);
>    5800                  if (ret < 0)
>    5801                          return ret;
>    5802          }
>    5803
>    5804          ret = 0;
>    5805          if (test_bit(EXTENT_BUFFER_UPTODATE, &eb->bflags) ||
>    5806              PageUptodate(page) ||
>    5807              btrfs_subpage_test_uptodate(fs_info, page, eb->start, eb->len)) {
>    5808                  set_bit(EXTENT_BUFFER_UPTODATE, &eb->bflags);
>    5809                  unlock_extent(io_tree, eb->start, eb->start + eb->len - 1);
>    5810                  return ret;
>    5811          }
>
> regards,
> dan carpenter
>

^ permalink raw reply	[flat|nested] 3+ messages in thread
* Re: [bug report] btrfs: introduce read_extent_buffer_subpage()
  2021-01-28 11:06 ` Qu Wenruo
@ 2021-01-28 11:10   ` Qu Wenruo
  0 siblings, 0 replies; 3+ messages in thread
From: Qu Wenruo @ 2021-01-28 11:10 UTC (permalink / raw)
  To: Dan Carpenter, wqu; +Cc: linux-btrfs



On 2021/1/28 下午7:06, Qu Wenruo wrote:
> 
> 
> On 2021/1/28 下午6:50, Dan Carpenter wrote:
>> Hello Qu Wenruo,
>>
>> The patch 5c60a522f1ea: "btrfs: introduce
>> read_extent_buffer_subpage()" from Jan 16, 2021, leads to the
>> following static checker warning:
>>
>>     fs/btrfs/extent_io.c:5797 read_extent_buffer_subpage()
>>     info: return a literal instead of 'ret'
>>
>> fs/btrfs/extent_io.c
>>    5780  static int read_extent_buffer_subpage(struct extent_buffer 
>> *eb, int wait,
>>    5781                                        int mirror_num)
>>    5782  {
>>    5783          struct btrfs_fs_info *fs_info = eb->fs_info;
>>    5784          struct extent_io_tree *io_tree;
>>    5785          struct page *page = eb->pages[0];
>>    5786          struct bio *bio = NULL;
>>    5787          int ret = 0;
>>    5788
>>    5789          ASSERT(!test_bit(EXTENT_BUFFER_UNMAPPED, &eb->bflags));
>>    5790          ASSERT(PagePrivate(page));
>>    5791          io_tree = &BTRFS_I(fs_info->btree_inode)->io_tree;
>>    5792
>>    5793          if (wait == WAIT_NONE) {
>>    5794                  ret = try_lock_extent(io_tree, eb->start,
>>    5795                                        eb->start + eb->len - 1);
>>    5796                  if (ret <= 0)
>>    5797                          return ret;
>>
>> If try_lock_extent() fails to get the lock and returns 0, then is
>> returning zero here really the correct behavior?
> 
> This is the same behavior of read_extent_buffer_pages() for regular
> sector size:
> 
> int read_extent_buffer_pages(struct extent_buffer *eb, int wait, int
> mirror_num)
> {
>      ...
>          int ret = 0;
>      ...
>              num_pages = num_extent_pages(eb);
>          for (i = 0; i < num_pages; i++) {
>                  page = eb->pages[i];
>                  if (wait == WAIT_NONE) {
>                          if (!trylock_page(page))
>                                  goto unlock_exit; <<<<
>      ...
> unlock_exit:
>          while (locked_pages > 0) {
>                  locked_pages--;
>                  page = eb->pages[locked_pages];
>                  unlock_page(page);
>          }
>          return ret;
> }
> 
> Here when we hit trylock_page() == false case, we directly go
> unlock_exit, and by that time, @ret is still 0.
> 
> 
> I'm not yet confident enough to say why it's OK, but my initial guess
> is, we won't have (wait == WAIT_NONE) case for metadata read.
> 
> Thank you for the hint, I'll take more time to make sure the original
> behavior is correct, and if it's really (wait == WAIT_NONE) will never
> be true for metadata, I'll send out cleanup for this.

Facepalm, I should check the code before hitting send.

The WAIT_NONE case is for readahead, thus we are completely fine not to 
read the tree block and just return 0.

For real tree reads, we always have WAIT_COMPLETE.

But still, I'll add some comment on the original code to explain why 
we're safe to return 0 directly if we can't lock the page directly.

Thanks,
Qu
> 
> Thanks,
> Qu
> 
>>  It feels like there
>> should be some documentation because this behavior is unexpected.
>>
>>    5798          } else {
>>    5799                  ret = lock_extent(io_tree, eb->start, 
>> eb->start + eb->len - 1);
>>    5800                  if (ret < 0)
>>    5801                          return ret;
>>    5802          }
>>    5803
>>    5804          ret = 0;
>>    5805          if (test_bit(EXTENT_BUFFER_UPTODATE, &eb->bflags) ||
>>    5806              PageUptodate(page) ||
>>    5807              btrfs_subpage_test_uptodate(fs_info, page, 
>> eb->start, eb->len)) {
>>    5808                  set_bit(EXTENT_BUFFER_UPTODATE, &eb->bflags);
>>    5809                  unlock_extent(io_tree, eb->start, eb->start + 
>> eb->len - 1);
>>    5810                  return ret;
>>    5811          }
>>
>> regards,
>> dan carpenter
>>

^ permalink raw reply	[flat|nested] 3+ messages in thread
end of thread, other threads:[~2021-01-28 11:12 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-01-28 10:50 [bug report] btrfs: introduce read_extent_buffer_subpage() Dan Carpenter
2021-01-28 11:06 ` Qu Wenruo
2021-01-28 11:10   ` Qu Wenruo

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.