Re: [PATCH] zonefs: use iomap for synchronous direct writes

[Damien Le Moal <dlemoal@kernel.org>]

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