public inbox for [email protected]
 help / color / mirror / Atom feed
From: John Garry <[email protected]>
To: Kanchan Joshi <[email protected]>,
	[email protected], [email protected], [email protected], [email protected],
	[email protected], [email protected],
	[email protected], [email protected], [email protected],
	[email protected]
Cc: [email protected], [email protected],
	[email protected], [email protected],
	[email protected], [email protected], [email protected],
	[email protected], [email protected],
	[email protected], [email protected],
	[email protected], [email protected],
	[email protected], [email protected], [email protected],
	[email protected], [email protected],
	[email protected], [email protected],
	Alan Adamson <[email protected]>
Subject: Re: [PATCH v8 10/10] nvme: Atomic write support
Date: Mon, 17 Jun 2024 19:04:23 +0100	[thread overview]
Message-ID: <[email protected]> (raw)
In-Reply-To: <[email protected]>

On 17/06/2024 18:24, Kanchan Joshi wrote:
> On 6/10/2024 4:13 PM, John Garry wrote:
>> +static bool nvme_valid_atomic_write(struct request *req)
>> +{
>> +	struct request_queue *q = req->q;
>> +	u32 boundary_bytes = queue_atomic_write_boundary_bytes(q);
>> +
>> +	if (blk_rq_bytes(req) > queue_atomic_write_unit_max_bytes(q))
>> +		return false;
>> +
>> +	if (boundary_bytes) {
>> +		u64 mask = boundary_bytes - 1, imask = ~mask;
>> +		u64 start = blk_rq_pos(req) << SECTOR_SHIFT;
>> +		u64 end = start + blk_rq_bytes(req) - 1;
>> +
>> +		/* If greater then must be crossing a boundary */
>> +		if (blk_rq_bytes(req) > boundary_bytes)
>> +			return false;
> 
> Nit: I'd cache blk_rq_bytes(req), since that is repeating and this
> function is called for each atomic IO.

blk_rq_bytes() is just a wrapper for rq->__data_len. I suppose that we 
could cache that value to stop re-reading that memory, but I would 
hope/expect that memory to be in the CPU cache anyway.

> 
>> +
>> +		if ((start & imask) != (end & imask))
>> +			return false;
>> +	}
>> +
>> +	return true;
>> +}
>> +
>>    static inline blk_status_t nvme_setup_rw(struct nvme_ns *ns,
>>    		struct request *req, struct nvme_command *cmnd,
>>    		enum nvme_opcode op)
>> @@ -941,6 +965,12 @@ static inline blk_status_t nvme_setup_rw(struct nvme_ns *ns,
>>    
>>    	if (req->cmd_flags & REQ_RAHEAD)
>>    		dsmgmt |= NVME_RW_DSM_FREQ_PREFETCH;
>> +	/*
>> +	 * Ensure that nothing has been sent which cannot be executed
>> +	 * atomically.
>> +	 */
>> +	if (req->cmd_flags & REQ_ATOMIC && !nvme_valid_atomic_write(req))
>> +		return BLK_STS_INVAL;
>>    
> 
> Is this validity check specific to NVMe or should this be moved up to
> block layer as it also knows the limits?

Only NVMe supports an LBA space boundary, so that part is specific to NVMe.

Regardless, the block layer already should ensure that the atomic write 
length and boundary is respected. nvme_valid_atomic_write() is just an 
insurance policy against the block layer or some other component not 
doing its job.

For SCSI, the device would error - for example - if the atomic write 
length was larger than the device supported. NVMe silently just does not 
execute the write atomically in that scenario, which we must avoid.

Thanks,
John


  reply	other threads:[~2024-06-17 18:05 UTC|newest]

Thread overview: 27+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2024-06-10 10:43 [PATCH v8 00/10] block atomic writes John Garry
2024-06-10 10:43 ` [PATCH v8 01/10] block: Pass blk_queue_get_max_sectors() a request pointer John Garry
2024-06-10 10:43 ` [PATCH v8 02/10] block: Generalize chunk_sectors support as boundary support John Garry
2024-06-10 10:43 ` [PATCH v8 03/10] fs: Initial atomic write support John Garry
2024-06-12 20:51   ` Darrick J. Wong
2024-06-10 10:43 ` [PATCH v8 04/10] fs: Add initial atomic write support info to statx John Garry
2024-06-12 20:54   ` Darrick J. Wong
2024-06-13  7:25     ` John Garry
2024-06-10 10:43 ` [PATCH v8 05/10] block: Add core atomic write support John Garry
2024-06-17 18:56   ` Keith Busch
2024-06-18  6:51     ` Christoph Hellwig
2024-06-18  7:46       ` John Garry
2024-06-18 17:25         ` Keith Busch
2024-06-19  7:59           ` John Garry
2024-06-19  8:02             ` Christoph Hellwig
2024-06-19 10:42               ` John Garry
2024-06-19 16:07               ` Martin K. Petersen
2024-06-10 10:43 ` [PATCH v8 06/10] block: Add atomic write support for statx John Garry
2024-06-10 10:43 ` [PATCH v8 07/10] block: Add fops atomic write support John Garry
2024-06-10 10:43 ` [PATCH v8 08/10] scsi: sd: Atomic " John Garry
2024-06-10 10:43 ` [PATCH v8 09/10] scsi: scsi_debug: " John Garry
2024-06-10 10:43 ` [PATCH v8 10/10] nvme: " John Garry
2024-06-17 17:24   ` Kanchan Joshi
2024-06-17 18:04     ` John Garry [this message]
2024-06-18  6:49       ` Christoph Hellwig
2024-06-18  7:22         ` John Garry
2024-06-14  2:01 ` [PATCH v8 00/10] block atomic writes Martin K. Petersen

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    [email protected] \
    [email protected] \
    [email protected] \
    [email protected] \
    [email protected] \
    [email protected] \
    [email protected] \
    [email protected] \
    [email protected] \
    [email protected] \
    [email protected] \
    [email protected] \
    [email protected] \
    [email protected] \
    [email protected] \
    [email protected] \
    [email protected] \
    [email protected] \
    [email protected] \
    [email protected] \
    [email protected] \
    [email protected] \
    [email protected] \
    [email protected] \
    [email protected] \
    [email protected] \
    [email protected] \
    [email protected] \
    [email protected] \
    [email protected] \
    [email protected] \
    [email protected] \
    [email protected] \
    [email protected] \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox