From: Damien Le Moal <damien.lemoal@opensource.wdc.com>
To: Chaitanya Kulkarni <kch@nvidia.com>,
linux-block@vger.kernel.org, linux-kernel@vger.kernel.org
Cc: axboe@kernel.dk, johannes.thumshirn@wdc.com, bvanassche@acm.org,
ming.lei@redhat.com, shinichiro.kawasaki@wdc.com,
vincent.fu@samsung.com, yukuai3@huawei.com
Subject: Re: [PATCH 3/6] null_blk: code cleaup
Date: Wed, 5 Oct 2022 14:02:38 +0900 [thread overview]
Message-ID: <b96c6d74-3436-c4c4-4c25-23960a1b3cc8@opensource.wdc.com> (raw)
In-Reply-To: <20221005031701.79077-4-kch@nvidia.com>
On 10/5/22 12:16, Chaitanya Kulkarni wrote:
> Introduce and use two new macros for calculating the page index from
> given sector and index (offset) of the sector in the page.
> The newly added macros makes code easy to read with meaningful name and
> explanation comments attached to it.
>
> While at it adjust the code in the null_free_sector() to return early
> to get rid of the extra identation.
>
> Signed-off-by: Chaitanya Kulkarni <kch@nvidia.com>
> ---
> drivers/block/null_blk/main.c | 37 ++++++++++++++++++++---------------
> 1 file changed, 21 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/block/null_blk/main.c b/drivers/block/null_blk/main.c
> index 2d592b4eb815..b82c2ffeb086 100644
> --- a/drivers/block/null_blk/main.c
> +++ b/drivers/block/null_blk/main.c
> @@ -14,6 +14,11 @@
> #undef pr_fmt
> #define pr_fmt(fmt) "null_blk: " fmt
>
> +/* Gives page index for which this sector belongs to. */
That is clear from the macro name. Not convinced this comment is useful.
> +#define PAGE_IDX_FROM_SECT(sect) (sect >> PAGE_SECTORS_SHIFT)
> +/* Gives index (offset) of the sector within page. */
> +#define SECT_IDX_IN_PAGE(sect) ((sect & SECTOR_MASK) << SECTOR_SHIFT)
SECT_OFFSET_IN_PAGE() ? A "page" not being an array of sectors, using
index for a sector is a little strange I think. And you use this macro for
things like:
offset = SECT_IDX_IN_PAGE(sect);
So offset in the name makes more sense.
> +
> #define FREE_BATCH 16
>
> #define TICKS_PER_SEC 50ULL
> @@ -860,20 +865,20 @@ static void null_free_sector(struct nullb *nullb, sector_t sector,
> struct radix_tree_root *root;
>
> root = is_cache ? &nullb->dev->cache : &nullb->dev->data;
> - idx = sector >> PAGE_SECTORS_SHIFT;
> + idx = PAGE_IDX_FROM_SECT(sector);
> sector_bit = (sector & SECTOR_MASK);
>
> t_page = radix_tree_lookup(root, idx);
> - if (t_page) {
> - __clear_bit(sector_bit, t_page->bitmap);
> -
> - if (null_page_empty(t_page)) {
> - ret = radix_tree_delete_item(root, idx, t_page);
> - WARN_ON(ret != t_page);
> - null_free_page(ret);
> - if (is_cache)
> - nullb->dev->curr_cache -= PAGE_SIZE;
> - }
> + if (!t_page)
> + return;
> + __clear_bit(sector_bit, t_page->bitmap);
> +
> + if (null_page_empty(t_page)) {
> + ret = radix_tree_delete_item(root, idx, t_page);
> + WARN_ON(ret != t_page);
> + null_free_page(ret);
> + if (is_cache)
> + nullb->dev->curr_cache -= PAGE_SIZE;
> }
> }
>
> @@ -885,11 +890,11 @@ static void null_zero_sector(struct nullb_device *d, sector_t sect,
> unsigned int offset;
> void *dest;
>
> - t_page = radix_tree_lookup(root, sect >> PAGE_SECTORS_SHIFT);
> + t_page = radix_tree_lookup(root, PAGE_IDX_FROM_SECT(sect));
> if (!t_page)
> return;
>
> - offset = (sect & SECTOR_MASK) << SECTOR_SHIFT;
> + offset = SECT_IDX_IN_PAGE(sect);
> dest = kmap_atomic(t_page->page);
> memset(dest + offset, 0, SECTOR_SIZE * nr_sects);
> kunmap_atomic(dest);
> @@ -949,7 +954,7 @@ static struct nullb_page *__null_lookup_page(struct nullb *nullb,
> struct nullb_page *t_page;
> struct radix_tree_root *root;
>
> - idx = sector >> PAGE_SECTORS_SHIFT;
> + idx = PAGE_IDX_FROM_SECT(sector);
> sector_bit = (sector & SECTOR_MASK);
>
> root = is_cache ? &nullb->dev->cache : &nullb->dev->data;
> @@ -1125,7 +1130,7 @@ static int copy_to_nullb(struct nullb *nullb, struct page *source,
> if (null_cache_active(nullb) && !is_fua)
> null_make_cache_space(nullb, PAGE_SIZE);
>
> - offset = (sector & SECTOR_MASK) << SECTOR_SHIFT;
> + offset = SECT_IDX_IN_PAGE(sector);
> t_page = null_insert_page(nullb, sector,
> !null_cache_active(nullb) || is_fua);
> if (!t_page)
> @@ -1159,7 +1164,7 @@ static int copy_from_nullb(struct nullb *nullb, struct page *dest,
> while (count < n) {
> temp = min_t(size_t, nullb->dev->blocksize, n - count);
>
> - offset = (sector & SECTOR_MASK) << SECTOR_SHIFT;
> + offset = SECT_IDX_IN_PAGE(sector);
> t_page = null_lookup_page(nullb, sector, false,
> !null_cache_active(nullb));
>
--
Damien Le Moal
Western Digital Research
next prev parent reply other threads:[~2022-10-05 5:03 UTC|newest]
Thread overview: 21+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-10-05 3:16 [PATCH 0/6] null_blk: allow REQ_OP_WRITE_ZEROES and cleanup Chaitanya Kulkarni
2022-10-05 3:16 ` [PATCH 1/6] null_blk: allow write zeores on non-membacked Chaitanya Kulkarni
2022-10-05 4:54 ` Damien Le Moal
2022-10-05 5:24 ` Chaitanya Kulkarni
2022-10-05 18:33 ` Chaitanya Kulkarni
2022-10-05 3:16 ` [PATCH 2/6] null_blk: allow write zeores on membacked Chaitanya Kulkarni
2022-10-05 4:57 ` Damien Le Moal
2022-10-05 5:10 ` Chaitanya Kulkarni
2022-10-05 17:18 ` Brian Foster
2022-10-05 18:45 ` Chaitanya Kulkarni
2022-10-05 18:53 ` Brian Foster
2022-10-05 19:01 ` Chaitanya Kulkarni
2022-10-05 3:16 ` [PATCH 3/6] null_blk: code cleaup Chaitanya Kulkarni
2022-10-05 5:02 ` Damien Le Moal [this message]
2022-10-05 5:21 ` Chaitanya Kulkarni
2022-10-05 3:16 ` [PATCH 4/6] null_blk: initialize cmd->bio in __alloc_cmd() Chaitanya Kulkarni
2022-10-05 5:04 ` Damien Le Moal
2022-10-05 3:17 ` [PATCH 5/6] null_blk: don't use magic numbers in the code Chaitanya Kulkarni
2022-10-05 5:05 ` Damien Le Moal
2022-10-05 3:17 ` [PATCH 6/6] null_blk: remove extra space in switch condition Chaitanya Kulkarni
2022-10-05 5:06 ` Damien Le Moal
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=b96c6d74-3436-c4c4-4c25-23960a1b3cc8@opensource.wdc.com \
--to=damien.lemoal@opensource.wdc.com \
--cc=axboe@kernel.dk \
--cc=bvanassche@acm.org \
--cc=johannes.thumshirn@wdc.com \
--cc=kch@nvidia.com \
--cc=linux-block@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=ming.lei@redhat.com \
--cc=shinichiro.kawasaki@wdc.com \
--cc=vincent.fu@samsung.com \
--cc=yukuai3@huawei.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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.