Re: [PATCH v20 12/12] null_blk: add support for copy offload

Nitesh Shetty
2024-05-22T05:01:51Z

[-- Attachment #1: Type: text/plain, Size: 3163 bytes --]

On 20/05/24 04:42PM, Bart Van Assche wrote:
>On 5/20/24 03:20, Nitesh Shetty wrote:
>>+ if (blk_rq_nr_phys_segments(req) != BLK_COPY_MAX_SEGMENTS)
>>+ return status;
>
>Why is this check necessary?
>
>>+ /*
>>+ * First bio contains information about destination and last bio
>>+ * contains information about source.
>>+ */
>
>Please check this at runtime (WARN_ON_ONCE()?).
>
>>+ __rq_for_each_bio(bio, req) {
>>+ if (seg == blk_rq_nr_phys_segments(req)) {
>>+ sector_in = bio->bi_iter.bi_sector;
>>+ if (rem != bio->bi_iter.bi_size)
>>+ return status;
>>+ } else {
>>+ sector_out = bio->bi_iter.bi_sector;
>>+ rem = bio->bi_iter.bi_size;
>>+ }
>>+ seg++;
>>+ }
>
>_rq_for_each_bio() iterates over the bios in a request. Does a copy
>offload request always have two bios - one copy destination bio and
>one copy source bio? If so, is 'seg' a bio counter? Why is that bio
>counter compared with the number of physical segments in the request?
>
Yes, your observation is right. We are treating first bio as dst and
second as src. If not for that comparision, we might need to store the
index in a temporary variable and parse based on index value.

>>+ trace_nullb_copy_op(req, sector_out << SECTOR_SHIFT,
>>+ sector_in << SECTOR_SHIFT, rem);
>>+
>>+ spin_lock_irq(&nullb->lock);
>>+ while (rem > 0) {
>>+ chunk = min_t(size_t, nullb->dev->blocksize, rem);
>>+ offset_in = (sector_in & SECTOR_MASK) << SECTOR_SHIFT;
>>+ offset_out = (sector_out & SECTOR_MASK) << SECTOR_SHIFT;
>>+
>>+ if (null_cache_active(nullb) && !is_fua)
>>+ null_make_cache_space(nullb, PAGE_SIZE);
>>+
>>+ t_page_in = null_lookup_page(nullb, sector_in, false,
>>+ !null_cache_active(nullb));
>>+ if (!t_page_in)
>>+ goto err;
>>+ t_page_out = null_insert_page(nullb, sector_out,
>>+ !null_cache_active(nullb) ||
>>+ is_fua);
>>+ if (!t_page_out)
>>+ goto err;
>>+
>>+ in = kmap_local_page(t_page_in->page);
>>+ out = kmap_local_page(t_page_out->page);
>>+
>>+ memcpy(out + offset_out, in + offset_in, chunk);
>>+ kunmap_local(out);
>>+ kunmap_local(in);
>>+ __set_bit(sector_out & SECTOR_MASK, t_page_out->bitmap);
>>+
>>+ if (is_fua)
>>+ null_free_sector(nullb, sector_out, true);
>>+
>>+ rem -= chunk;
>>+ sector_in += chunk >> SECTOR_SHIFT;
>>+ sector_out += chunk >> SECTOR_SHIFT;
>>+ }
>>+
>>+ status = 0;
>>+err:
>>+ spin_unlock_irq(&nullb->lock);
>
>In the worst case, how long does this loop disable interrupts?
>
We havn't measured this. But this should be similar to read and write in
present infra, as we followed similar approach.

>>+TRACE_EVENT(nullb_copy_op,
>>+ TP_PROTO(struct request *req,
>>+ sector_t dst, sector_t src, size_t len),
>>+ TP_ARGS(req, dst, src, len),
>>+ TP_STRUCT__entry(
>>+ __array(char, disk, DISK_NAME_LEN)
>>+ __field(enum req_op, op)
>>+ __field(sector_t, dst)
>>+ __field(sector_t, src)
>>+ __field(size_t, len)
>>+ ),
>
>Isn't __string() preferred over __array() since the former occupies less space
>in the trace buffer?
>
Again we followed the present existing implementation, to have a simpler
series to review.

Thank you,
Nitesh Shetty

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]