On 6/1/23 18:27, Christoph Hellwig wrote:
>> +struct zonefs_bio {
>> + /* The target inode of the BIO */
>> + struct inode *inode;
>> +
>> + /* For sync writes, the target write offset */
>> + u64 woffset;
>
> Maybe spell out write_offset?
Yep. will do as I need to send a v2 anyway as I am seeing random test failure of
the case handling writes to offline zones. Not sure why. Debugging.
>
>> +
>> +static void zonefs_file_sync_write_dio_bio_end_io(struct bio *bio)
>> +{
>> + struct zonefs_bio *zbio;
>> + struct zonefs_zone *z;
>> + sector_t wsector;
>> +
>> + if (bio->bi_status != BLK_STS_OK)
>> + goto bio_end;
>> +
>> + /*
>> + * If the file zone was written underneath the file system, the zone
>> + * append operation can still succedd (if the zone is not full) but
>> + * the write append location will not be where we expect it to be.
>> + * Check that we wrote where we intended to, that is, at z->z_wpoffset.
>> + */
>> + zbio = zonefs_bio(bio);
>> + z = zonefs_inode_zone(zbio->inode);
>
> I'd move thses to the lines where the variables are declared.
OK
>
>> + wsector = z->z_sector + (zbio->woffset >> SECTOR_SHIFT);
>> + if (bio->bi_iter.bi_sector != wsector) {
>> + zonefs_warn(zbio->inode->i_sb,
>> + "Invalid write sector %llu for zone at %llu\n",
>> + bio->bi_iter.bi_sector, z->z_sector);
>> + bio->bi_status = BLK_STS_IOERR;
>> + }
>
> Seems like all this is actually just debug code and could be conditional
> and you could just use the normal bio pool otherwise?
Without this, the zonefs_bio struct and bio_end_io op are not even needed. That
would simplify things but I really would like to keep this around to be able to
detect file corruptions.
>
>> +static struct bio_set zonefs_file_write_dio_bio_set;
>>
--
Damien Le Moal
Western Digital Research
