| Message ID | 20201217045737.48100-3-wqu@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | btrfs: inode: btrfs_invalidatepage() related refactor and fix for subpage | expand |
On Thu 17 Dec 2020 at 12:57, Qu Wenruo <wqu@suse.com> wrote: > In btrfs_invalidatepage() we re-declare @tree variable as > btrfs_ordered_inode_tree. > > Remove such variable shadowing which can be very confusing. > > Signed-off-by: Qu Wenruo <wqu@suse.com> > --- > fs/btrfs/inode.c | 9 +++------ > 1 file changed, 3 insertions(+), 6 deletions(-) > > diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c > index dced71bccaac..b4d36d138008 100644 > --- a/fs/btrfs/inode.c > +++ b/fs/btrfs/inode.c > @@ -8169,6 +8169,7 @@ static void btrfs_invalidatepage(struct > page *page, unsigned int offset, > unsigned int length) > { > struct btrfs_inode *inode = BTRFS_I(page->mapping->host); > + struct btrfs_ordered_inode_tree *ordered_tree = > &inode->ordered_tree; > Any reason for the declaration here? I didn't find that patch[3/4] use it. > struct extent_io_tree *tree = &inode->io_tree; > struct btrfs_ordered_extent *ordered; > struct extent_state *cached_state = NULL; > @@ -8218,15 +8219,11 @@ static void btrfs_invalidatepage(struct > page *page, unsigned int offset, > * for the finish_ordered_io > */ > if (TestClearPagePrivate2(page)) { > - struct btrfs_ordered_inode_tree *tree; > - Better to just rename the @tree to @ordered_tree. > - tree = &inode->ordered_tree; > - > - spin_lock_irq(&tree->lock); > + spin_lock_irq(&ordered_tree->lock); > set_bit(BTRFS_ORDERED_TRUNCATED, &ordered->flags); > ordered->truncated_len = min(ordered->truncated_len, > start - ordered->file_offset); > - spin_unlock_irq(&tree->lock); > + spin_unlock_irq(&ordered_tree->lock); > > ASSERT(end - start + 1 < U32_MAX); > if (btrfs_dec_test_ordered_pending(inode, &ordered,
On 2020/12/17 下午1:38, Su Yue wrote: > > On Thu 17 Dec 2020 at 12:57, Qu Wenruo <wqu@suse.com> wrote: > >> In btrfs_invalidatepage() we re-declare @tree variable as >> btrfs_ordered_inode_tree. >> >> Remove such variable shadowing which can be very confusing. >> >> Signed-off-by: Qu Wenruo <wqu@suse.com> >> --- >> fs/btrfs/inode.c | 9 +++------ >> 1 file changed, 3 insertions(+), 6 deletions(-) >> >> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c >> index dced71bccaac..b4d36d138008 100644 >> --- a/fs/btrfs/inode.c >> +++ b/fs/btrfs/inode.c >> @@ -8169,6 +8169,7 @@ static void btrfs_invalidatepage(struct page >> *page, unsigned int offset, >> unsigned int length) >> { >> struct btrfs_inode *inode = BTRFS_I(page->mapping->host); >> + struct btrfs_ordered_inode_tree *ordered_tree = >> &inode->ordered_tree; >> > Any reason for the declaration here? I didn't find that patch[3/4] use it. Didn't that ordered_tree get used lines below? > >> struct extent_io_tree *tree = &inode->io_tree; >> struct btrfs_ordered_extent *ordered; >> struct extent_state *cached_state = NULL; >> @@ -8218,15 +8219,11 @@ static void btrfs_invalidatepage(struct page >> *page, unsigned int offset, >> * for the finish_ordered_io >> */ >> if (TestClearPagePrivate2(page)) { >> - struct btrfs_ordered_inode_tree *tree; >> - > Better to just rename the @tree to @ordered_tree. Isn't that exactly what I did? Thanks, Qu > >> - tree = &inode->ordered_tree; >> - >> - spin_lock_irq(&tree->lock); >> + spin_lock_irq(&ordered_tree->lock); >> set_bit(BTRFS_ORDERED_TRUNCATED, &ordered->flags); >> ordered->truncated_len = min(ordered->truncated_len, >> start - ordered->file_offset); >> - spin_unlock_irq(&tree->lock); >> + spin_unlock_irq(&ordered_tree->lock); >> >> ASSERT(end - start + 1 < U32_MAX); >> if (btrfs_dec_test_ordered_pending(inode, &ordered, >
On 17.12.20 г. 6:57 ч., Qu Wenruo wrote: > In btrfs_invalidatepage() we re-declare @tree variable as > btrfs_ordered_inode_tree. > > Remove such variable shadowing which can be very confusing. You can't do that, because lock_extent_bits expects extent_io_tree !
On 17.12.20 г. 7:55 ч., Nikolay Borisov wrote: > > > On 17.12.20 г. 6:57 ч., Qu Wenruo wrote: >> In btrfs_invalidatepage() we re-declare @tree variable as >> btrfs_ordered_inode_tree. >> >> Remove such variable shadowing which can be very confusing. > > You can't do that, because lock_extent_bits expects extent_io_tree ! > Ok, nvm, you just factored the var at the beginning of the functions. OTOH since the ordered tree is used just for lock/unlock why not do spin_(un)lock(&inode->ordered_tree->lock);
On Thu 17 Dec 2020 at 13:42, Qu Wenruo <quwenruo.btrfs@gmx.com> wrote: > On 2020/12/17 下午1:38, Su Yue wrote: >> >> On Thu 17 Dec 2020 at 12:57, Qu Wenruo <wqu@suse.com> wrote: >> >>> In btrfs_invalidatepage() we re-declare @tree variable as >>> btrfs_ordered_inode_tree. >>> >>> Remove such variable shadowing which can be very confusing. >>> >>> Signed-off-by: Qu Wenruo <wqu@suse.com> >>> --- >>> fs/btrfs/inode.c | 9 +++------ >>> 1 file changed, 3 insertions(+), 6 deletions(-) >>> >>> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c >>> index dced71bccaac..b4d36d138008 100644 >>> --- a/fs/btrfs/inode.c >>> +++ b/fs/btrfs/inode.c >>> @@ -8169,6 +8169,7 @@ static void btrfs_invalidatepage(struct >>> page >>> *page, unsigned int offset, >>> unsigned int length) >>> { >>> struct btrfs_inode *inode = BTRFS_I(page->mapping->host); >>> + struct btrfs_ordered_inode_tree *ordered_tree = >>> &inode->ordered_tree; >>> >> Any reason for the declaration here? I didn't find that >> patch[3/4] use it. > > Didn't that ordered_tree get used lines below? > >> >>> struct extent_io_tree *tree = &inode->io_tree; >>> struct btrfs_ordered_extent *ordered; >>> struct extent_state *cached_state = NULL; >>> @@ -8218,15 +8219,11 @@ static void >>> btrfs_invalidatepage(struct page >>> *page, unsigned int offset, >>> * for the finish_ordered_io >>> */ >>> if (TestClearPagePrivate2(page)) { >>> - struct btrfs_ordered_inode_tree *tree; >>> - >> Better to just rename the @tree to @ordered_tree. > > Isn't that exactly what I did? What I mean is that keep the declaration in the block since no further use of it. > > Thanks, > Qu >> >>> - tree = &inode->ordered_tree; >>> - >>> - spin_lock_irq(&tree->lock); >>> + spin_lock_irq(&ordered_tree->lock); >>> set_bit(BTRFS_ORDERED_TRUNCATED, >>> &ordered->flags); >>> ordered->truncated_len = >>> min(ordered->truncated_len, >>> start - ordered->file_offset); >>> - spin_unlock_irq(&tree->lock); >>> + spin_unlock_irq(&ordered_tree->lock); >>> >>> ASSERT(end - start + 1 < U32_MAX); >>> if (btrfs_dec_test_ordered_pending(inode, >>> &ordered, >>
On 2020/12/17 下午1:59, Nikolay Borisov wrote: > > > On 17.12.20 г. 7:55 ч., Nikolay Borisov wrote: >> >> >> On 17.12.20 г. 6:57 ч., Qu Wenruo wrote: >>> In btrfs_invalidatepage() we re-declare @tree variable as >>> btrfs_ordered_inode_tree. >>> >>> Remove such variable shadowing which can be very confusing. >> >> You can't do that, because lock_extent_bits expects extent_io_tree ! >> > > Ok, nvm, you just factored the var at the beginning of the functions. > OTOH since the ordered tree is used just for lock/unlock why not do > spin_(un)lock(&inode->ordered_tree->lock); > Oh, that indeed looks better and since Su is also complaining about the declaration at the beginning of the function, I guess that's the better way to go. Thanks, Qu
On Thu, Dec 17, 2020 at 02:13:37PM +0800, Qu Wenruo wrote: > > > On 2020/12/17 下午1:59, Nikolay Borisov wrote: > > > > > > On 17.12.20 г. 7:55 ч., Nikolay Borisov wrote: > >> > >> > >> On 17.12.20 г. 6:57 ч., Qu Wenruo wrote: > >>> In btrfs_invalidatepage() we re-declare @tree variable as > >>> btrfs_ordered_inode_tree. > >>> > >>> Remove such variable shadowing which can be very confusing. > >> > >> You can't do that, because lock_extent_bits expects extent_io_tree ! > >> > > > > Ok, nvm, you just factored the var at the beginning of the functions. > > OTOH since the ordered tree is used just for lock/unlock why not do > > spin_(un)lock(&inode->ordered_tree->lock); > > > Oh, that indeed looks better and since Su is also complaining about the > declaration at the beginning of the function, I guess that's the better > way to go. The preferred style is to declare variables in the closest scope, so there's not a huge blob of declarations that are randomly used inside nested blocks. It's more like a recommendation, eg. when the function is short and there are a few variables used inside a for/while.
diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c index dced71bccaac..b4d36d138008 100644 --- a/fs/btrfs/inode.c +++ b/fs/btrfs/inode.c @@ -8169,6 +8169,7 @@ static void btrfs_invalidatepage(struct page *page, unsigned int offset, unsigned int length) { struct btrfs_inode *inode = BTRFS_I(page->mapping->host); + struct btrfs_ordered_inode_tree *ordered_tree = &inode->ordered_tree; struct extent_io_tree *tree = &inode->io_tree; struct btrfs_ordered_extent *ordered; struct extent_state *cached_state = NULL; @@ -8218,15 +8219,11 @@ static void btrfs_invalidatepage(struct page *page, unsigned int offset, * for the finish_ordered_io */ if (TestClearPagePrivate2(page)) { - struct btrfs_ordered_inode_tree *tree; - - tree = &inode->ordered_tree; - - spin_lock_irq(&tree->lock); + spin_lock_irq(&ordered_tree->lock); set_bit(BTRFS_ORDERED_TRUNCATED, &ordered->flags); ordered->truncated_len = min(ordered->truncated_len, start - ordered->file_offset); - spin_unlock_irq(&tree->lock); + spin_unlock_irq(&ordered_tree->lock); ASSERT(end - start + 1 < U32_MAX); if (btrfs_dec_test_ordered_pending(inode, &ordered,
In btrfs_invalidatepage() we re-declare @tree variable as btrfs_ordered_inode_tree. Remove such variable shadowing which can be very confusing. Signed-off-by: Qu Wenruo <wqu@suse.com> --- fs/btrfs/inode.c | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-)