Re: [RFC v2 00/11] dm-pcache – persistent-memory cache for block devices

[Mikulas Patocka <mpatocka@redhat.com>]

On Mon, 23 Jun 2025, Dongsheng Yang wrote:

> +static int dm_pcache_map_bio(struct dm_target *ti, struct bio *bio)
> +{
> +     struct pcache_request *pcache_req = dm_per_bio_data(bio, sizeof(struct pcache_request));
> +     struct dm_pcache *pcache = ti->private;
> +     int ret;
> +
> +     pcache_req->pcache = pcache;
> +     kref_init(&pcache_req->ref);
> +     pcache_req->ret = 0;
> +     pcache_req->bio = bio;
> +     pcache_req->off = (u64)bio->bi_iter.bi_sector << SECTOR_SHIFT;
> +     pcache_req->data_len = bio->bi_iter.bi_size;
> +     INIT_LIST_HEAD(&pcache_req->list_node);
> +     bio->bi_iter.bi_sector = dm_target_offset(ti, bio->bi_iter.bi_sector);
> 
> This looks suspicious because you store the original bi_sector to
> pcache_req->off and then subtract the target offset from it. Shouldn't
> "bio->bi_iter.bi_sector = dm_target_offset(ti, bio->bi_iter.bi_sector);"
> be before "pcache_req->off = (u64)bio->bi_iter.bi_sector << 
> SECTOR_SHIFT;"?
> 
> 
> Yes, that logic is indeed questionable, but it works in testing.
> 
> Since we define dm-pcache as a **singleton**, both behaviors should 
> effectively be equivalent, IIUC. Also, in V1 I moved the call to 
> `dm_target_offset()` so it runs before setting up `pcache_req->off`, 
> making the code logic correct.

If this target is singleton, you can delete the call to dm_target_offset 
at all.

That call is harmless, but it looks confusing when reviewing the code, 
because pcache_req->off is set to the absolute bio sector (from the start 
of the table) and bio->bi_iter.bi_sector is set to the relative bio sector 
(from the start of the target). If the target always starts at offset 0, 
dm_target_offset just returns bi_sector.

Mikulas