| Message ID | 1515766865-13652-1-git-send-email-nborisov@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 01/12/2018 07:21 AM, Nikolay Borisov wrote: > The behavior of btrfs_delalloc_reserve_metadata depends on whether > the inode we are allocating for is the freespace inode or not. As it > stands if we are the free node we set 'flush' and 'delalloc_lock' > variable to certain values. Subsequently we check the values of those > vars and act accordingly. Instead, simplify things by having 1 if > which checks whether we are the freespace inode or not and do any > specific operation in either branches of that if. This makes the code > a bit easier to understand, as an added bonus it also shrinks the > compiled size: > > add/remove: 0/0 grow/shrink: 0/1 up/down: 0/-17 (-17) > Function old new delta > btrfs_delalloc_reserve_metadata 1876 1859 -17 > Total: Before=85966, After=85949, chg -0.02% > > No functional changes. > > Signed-off-by: Nikolay Borisov <nborisov@suse.com> Reviewed-by: Edmund Nadolski <enadolski@suse.com> > --- > fs/btrfs/extent-tree.c | 14 +++++++------- > 1 file changed, 7 insertions(+), 7 deletions(-) > > diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c > index 8d51e4bb67c1..47295804d91d 100644 > --- a/fs/btrfs/extent-tree.c > +++ b/fs/btrfs/extent-tree.c > @@ -6062,19 +6062,19 @@ int btrfs_delalloc_reserve_metadata(struct btrfs_inode *inode, u64 num_bytes) > * If we have a transaction open (can happen if we call truncate_block > * from truncate), then we need FLUSH_LIMIT so we don't deadlock. > */ > + > if (btrfs_is_free_space_inode(inode)) { > flush = BTRFS_RESERVE_NO_FLUSH; > delalloc_lock = false; > - } else if (current->journal_info) { > - flush = BTRFS_RESERVE_FLUSH_LIMIT; > - } > + } else { > + if (current->journal_info) > + flush = BTRFS_RESERVE_FLUSH_LIMIT; > > - if (flush != BTRFS_RESERVE_NO_FLUSH && > - btrfs_transaction_in_commit(fs_info)) > - schedule_timeout(1); > + if (btrfs_transaction_in_commit(fs_info)) > + schedule_timeout(1); > > - if (delalloc_lock) > mutex_lock(&inode->delalloc_mutex); > + } > > num_bytes = ALIGN(num_bytes, fs_info->sectorsize); > > -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Jan 12, 2018 at 04:21:05PM +0200, Nikolay Borisov wrote: > The behavior of btrfs_delalloc_reserve_metadata depends on whether > the inode we are allocating for is the freespace inode or not. As it > stands if we are the free node we set 'flush' and 'delalloc_lock' > variable to certain values. Subsequently we check the values of those > vars and act accordingly. Instead, simplify things by having 1 if > which checks whether we are the freespace inode or not and do any > specific operation in either branches of that if. This makes the code > a bit easier to understand, as an added bonus it also shrinks the > compiled size: > > add/remove: 0/0 grow/shrink: 0/1 up/down: 0/-17 (-17) > Function old new delta > btrfs_delalloc_reserve_metadata 1876 1859 -17 > Total: Before=85966, After=85949, chg -0.02% This looks too fine grained and IMHO not useful to mention. The overall module size delta is interesting when compared between the base nad pull request, but not for individual patches, namely if it's just 17 bytes. -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Jan 12, 2018 at 04:21:05PM +0200, Nikolay Borisov wrote: > @@ -6062,19 +6062,19 @@ int btrfs_delalloc_reserve_metadata(struct btrfs_inode *inode, u64 num_bytes) > * If we have a transaction open (can happen if we call truncate_block > * from truncate), then we need FLUSH_LIMIT so we don't deadlock. > */ > + > if (btrfs_is_free_space_inode(inode)) { > flush = BTRFS_RESERVE_NO_FLUSH; > delalloc_lock = false; > - } else if (current->journal_info) { > - flush = BTRFS_RESERVE_FLUSH_LIMIT; > - } > + } else { > + if (current->journal_info) > + flush = BTRFS_RESERVE_FLUSH_LIMIT; > > - if (flush != BTRFS_RESERVE_NO_FLUSH && > - btrfs_transaction_in_commit(fs_info)) > - schedule_timeout(1); > + if (btrfs_transaction_in_commit(fs_info)) > + schedule_timeout(1); > > - if (delalloc_lock) > mutex_lock(&inode->delalloc_mutex); > + } Squeezing the condition branches makes the code more readable, I have only one objection and it's the mutex_lock. It IMHO looks better when it's a separate branch as it pairs with the unlock: if (delalloc_lock) mutex_lock(...); ... if (delalloc_lock) mutex_unlock(...); In your version it's implied by the first if that checks btrfs_is_free_space_inode and delalloc_lock is hidden there. > > num_bytes = ALIGN(num_bytes, fs_info->sectorsize); -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 30.01.2018 16:34, David Sterba wrote: > On Fri, Jan 12, 2018 at 04:21:05PM +0200, Nikolay Borisov wrote: >> @@ -6062,19 +6062,19 @@ int btrfs_delalloc_reserve_metadata(struct btrfs_inode *inode, u64 num_bytes) >> * If we have a transaction open (can happen if we call truncate_block >> * from truncate), then we need FLUSH_LIMIT so we don't deadlock. >> */ >> + >> if (btrfs_is_free_space_inode(inode)) { >> flush = BTRFS_RESERVE_NO_FLUSH; >> delalloc_lock = false; >> - } else if (current->journal_info) { >> - flush = BTRFS_RESERVE_FLUSH_LIMIT; >> - } >> + } else { >> + if (current->journal_info) >> + flush = BTRFS_RESERVE_FLUSH_LIMIT; >> >> - if (flush != BTRFS_RESERVE_NO_FLUSH && >> - btrfs_transaction_in_commit(fs_info)) >> - schedule_timeout(1); >> + if (btrfs_transaction_in_commit(fs_info)) >> + schedule_timeout(1); >> >> - if (delalloc_lock) >> mutex_lock(&inode->delalloc_mutex); >> + } > > Squeezing the condition branches makes the code more readable, I have > only one objection and it's the mutex_lock. It IMHO looks better when > it's a separate branch as it pairs with the unlock: > > if (delalloc_lock) > mutex_lock(...); > > ... > > if (delalloc_lock) > mutex_unlock(...); > > In your version it's implied by the first if that checks > btrfs_is_free_space_inode and delalloc_lock is hidden there. > My line of thought when developing the patch was that delalloc is another level of indirection and. What I wanted to achieve in the end is to make it clear that delalloc_mutex really depends on whether we are reserving for the freespace inode or not. >> >> num_bytes = ALIGN(num_bytes, fs_info->sectorsize); -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Jan 30, 2018 at 04:47:54PM +0200, Nikolay Borisov wrote: > On 30.01.2018 16:34, David Sterba wrote: > > On Fri, Jan 12, 2018 at 04:21:05PM +0200, Nikolay Borisov wrote: > >> @@ -6062,19 +6062,19 @@ int btrfs_delalloc_reserve_metadata(struct btrfs_inode *inode, u64 num_bytes) > >> * If we have a transaction open (can happen if we call truncate_block > >> * from truncate), then we need FLUSH_LIMIT so we don't deadlock. > >> */ > >> + > >> if (btrfs_is_free_space_inode(inode)) { > >> flush = BTRFS_RESERVE_NO_FLUSH; > >> delalloc_lock = false; > >> - } else if (current->journal_info) { > >> - flush = BTRFS_RESERVE_FLUSH_LIMIT; > >> - } > >> + } else { > >> + if (current->journal_info) > >> + flush = BTRFS_RESERVE_FLUSH_LIMIT; > >> > >> - if (flush != BTRFS_RESERVE_NO_FLUSH && > >> - btrfs_transaction_in_commit(fs_info)) > >> - schedule_timeout(1); > >> + if (btrfs_transaction_in_commit(fs_info)) > >> + schedule_timeout(1); > >> > >> - if (delalloc_lock) > >> mutex_lock(&inode->delalloc_mutex); > >> + } > > > > Squeezing the condition branches makes the code more readable, I have > > only one objection and it's the mutex_lock. It IMHO looks better when > > it's a separate branch as it pairs with the unlock: > > > > if (delalloc_lock) > > mutex_lock(...); > > > > ... > > > > if (delalloc_lock) > > mutex_unlock(...); > > > > In your version it's implied by the first if that checks > > btrfs_is_free_space_inode and delalloc_lock is hidden there. > > > > My line of thought when developing the patch was that delalloc is > another level of indirection and. What I wanted to achieve in the end is > to make it clear that delalloc_mutex really depends on whether we are > reserving for the freespace inode or not. Well, I almost overlooked the mutex on first top-down reading the code. -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c index 8d51e4bb67c1..47295804d91d 100644 --- a/fs/btrfs/extent-tree.c +++ b/fs/btrfs/extent-tree.c @@ -6062,19 +6062,19 @@ int btrfs_delalloc_reserve_metadata(struct btrfs_inode *inode, u64 num_bytes) * If we have a transaction open (can happen if we call truncate_block * from truncate), then we need FLUSH_LIMIT so we don't deadlock. */ + if (btrfs_is_free_space_inode(inode)) { flush = BTRFS_RESERVE_NO_FLUSH; delalloc_lock = false; - } else if (current->journal_info) { - flush = BTRFS_RESERVE_FLUSH_LIMIT; - } + } else { + if (current->journal_info) + flush = BTRFS_RESERVE_FLUSH_LIMIT; - if (flush != BTRFS_RESERVE_NO_FLUSH && - btrfs_transaction_in_commit(fs_info)) - schedule_timeout(1); + if (btrfs_transaction_in_commit(fs_info)) + schedule_timeout(1); - if (delalloc_lock) mutex_lock(&inode->delalloc_mutex); + } num_bytes = ALIGN(num_bytes, fs_info->sectorsize);
The behavior of btrfs_delalloc_reserve_metadata depends on whether the inode we are allocating for is the freespace inode or not. As it stands if we are the free node we set 'flush' and 'delalloc_lock' variable to certain values. Subsequently we check the values of those vars and act accordingly. Instead, simplify things by having 1 if which checks whether we are the freespace inode or not and do any specific operation in either branches of that if. This makes the code a bit easier to understand, as an added bonus it also shrinks the compiled size: add/remove: 0/0 grow/shrink: 0/1 up/down: 0/-17 (-17) Function old new delta btrfs_delalloc_reserve_metadata 1876 1859 -17 Total: Before=85966, After=85949, chg -0.02% No functional changes. Signed-off-by: Nikolay Borisov <nborisov@suse.com> --- fs/btrfs/extent-tree.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-)