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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <bf733634-cb99-48db-9b02-09b9700cd1f0@linux.dev>
Date: Tue, 1 Jul 2025 00:28:03 +0800
From: Dongsheng Yang <dongsheng.yang@...ux.dev>
To: Mikulas Patocka <mpatocka@...hat.com>
Cc: agk@...hat.com, snitzer@...nel.org, axboe@...nel.dk, hch@....de,
 dan.j.williams@...el.com, Jonathan.Cameron@...wei.com,
 linux-block@...r.kernel.org, linux-kernel@...r.kernel.org,
 linux-cxl@...r.kernel.org, nvdimm@...ts.linux.dev, dm-devel@...ts.linux.dev
Subject: Re: [RFC v2 00/11] dm-pcache – persistent-memory cache for block devices


在 6/30/2025 11:57 PM, Mikulas Patocka 写道:
>
> 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.


That makes sense

Thanx

>
> Mikulas
>