From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-pf1-f169.google.com (mail-pf1-f169.google.com [209.85.210.169]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 3DA0154FA7 for ; Mon, 19 Feb 2024 22:58:43 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=209.85.210.169 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1708383526; cv=none; b=OQXM8oOVPmu5v1GKgSxyh2/MbjTXj7mbPseis3FhjvElaBqzM4QmK32NHAAqVQLgX3gyrTt88Uqtm5ZM8OffXkc8D/blzj3E+7kOrw7q5H/cYw/Yss4YIilU0iXfA2bq6egvqrjvsNpRpxLPPiGB44frAAgn72ZHeqha7fwvBDI= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1708383526; c=relaxed/simple; bh=2SNzo6Nm6HcU5wL9JoMRvctOw4lKDTA8DZJYggBpUos=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=ZcO7FKePbh2CrpqQEtXpIYMXnK7+HulDv/SIL15q8x4qTdF7Odsq+OAejJCx46Y7MJRPPz1IKStTg0P4NamDLfiwAcNohJ0WPrjMHhDLEmKme71t+FeUZrIXMbCpwGfRsuL+yp+bnCGe+CiQjRecGvM+BQvDON3Ck/BApmw809U= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=quarantine dis=none) header.from=fromorbit.com; spf=pass smtp.mailfrom=fromorbit.com; dkim=pass (2048-bit key) header.d=fromorbit-com.20230601.gappssmtp.com header.i=@fromorbit-com.20230601.gappssmtp.com header.b=IrbhwGQd; arc=none smtp.client-ip=209.85.210.169 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=quarantine dis=none) header.from=fromorbit.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=fromorbit.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=fromorbit-com.20230601.gappssmtp.com header.i=@fromorbit-com.20230601.gappssmtp.com header.b="IrbhwGQd" Received: by mail-pf1-f169.google.com with SMTP id d2e1a72fcca58-6e08dd0fa0bso4118970b3a.1 for ; Mon, 19 Feb 2024 14:58:43 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=fromorbit-com.20230601.gappssmtp.com; s=20230601; t=1708383522; x=1708988322; darn=vger.kernel.org; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:from:to:cc:subject:date:message-id:reply-to; bh=G2SusDOlkkZCINXkEP6RVbB2J47VRPBecZ6j24NRkMs=; b=IrbhwGQd+TUNReUTruXWKe7va0WBiuoxnX+sEsKPE28yTn8dzJiQmj+hIAnhV34uu7 ZJtpzEyYXP8kr0HMS+/Efjwhu4UJuXp0tI+iH4MSXaXK35vP1K5T92CkdLzrOk7tTOWL gNfWCCRW1w8i8pO28muJTdxx66IddHiCNFwTWWNUMC7boNk55tGkwMgqh7q0XwbCyOr0 oTzvsVcoWooLn+Rhjr24VKnwfWSewV0MfDwOzfB/TItTFiYJVaratRLTIexShDOh47OE r+WQjdXQF9c1yCi7Pxbgz5GZE402irit/i1VzQ7iidROnXBGOt0xrnssYzTMkkica29s NszA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1708383522; x=1708988322; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:x-gm-message-state:from:to:cc:subject:date :message-id:reply-to; bh=G2SusDOlkkZCINXkEP6RVbB2J47VRPBecZ6j24NRkMs=; b=uKCPMdIcftWBaChce49hv85D6sDYxXmJgufMm5rNdqrDFCCIz2rBcX4nPaLHqmAqYU IuCjAeFjVI73yWXAfq6tQRMjU9KLYikA+/UNxk8WhJb+V/OAbuHLgUrHuYMaqKuC9QlA l2hVgkyYI/JDrInWuuvC0kMUmLPW3zqCxG6jsa33myuRqo2xBl8eZYlu3i6hS9HenkbR Rna8Lj59WW0YY5Qv16gcJ7SxbI9znLJaR+avqP8UfmbQBTAk5NFqHOw86AUmU0PGYU2X 5xSDUWl0j+vilK189SmLUuujnVqeSQ0HFFDA5oRBVGjwBkRWnL7anfTs91UcXhrigaHe mnPw== X-Forwarded-Encrypted: i=1; AJvYcCWQZxAmsrBT+7FTqBHG5rX9vUdSQmxJYsQCXgiCYHsj32ZKD8OWK5fUkkiCGEXflIHHi9gbbyFmZhOUavi356lJiFi7n9qnFo0= X-Gm-Message-State: AOJu0YyO9FYCnRn572YvZ4s0i+E3seAMWuPpm8Jw/+XpUCImJZbxCBol wYDet+UlBLQBRfMGTJtAUjZd3qsEDWM6/fGb8R5DW4GaE08K2R3ARxWhkA5kgtM= X-Google-Smtp-Source: AGHT+IHlfzy+s9i3qKJWzkHasWDQ5Dp8s/ZMPkMUlAlAKBlL38U+vY5Udn0W8KhJ2rlPYO9vcrxajg== X-Received: by 2002:a05:6a00:198f:b0:6e4:74d7:a732 with SMTP id d15-20020a056a00198f00b006e474d7a732mr2108349pfl.11.1708383522593; Mon, 19 Feb 2024 14:58:42 -0800 (PST) Received: from dread.disaster.area (pa49-181-247-196.pa.nsw.optusnet.com.au. [49.181.247.196]) by smtp.gmail.com with ESMTPSA id e8-20020a62ee08000000b006e02da39dbcsm3319334pfi.10.2024.02.19.14.58.42 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 19 Feb 2024 14:58:42 -0800 (PST) Received: from dave by dread.disaster.area with local (Exim 4.96) (envelope-from ) id 1rcCat-008oe1-1J; Tue, 20 Feb 2024 09:58:39 +1100 Date: Tue, 20 Feb 2024 09:58:39 +1100 From: Dave Chinner To: John Garry Cc: axboe@kernel.dk, kbusch@kernel.org, hch@lst.de, sagi@grimberg.me, jejb@linux.ibm.com, martin.petersen@oracle.com, djwong@kernel.org, viro@zeniv.linux.org.uk, brauner@kernel.org, dchinner@redhat.com, jack@suse.cz, linux-block@vger.kernel.org, linux-kernel@vger.kernel.org, linux-nvme@lists.infradead.org, linux-fsdevel@vger.kernel.org, tytso@mit.edu, jbongio@google.com, linux-scsi@vger.kernel.org, ojaswin@linux.ibm.com, linux-aio@kvack.org, linux-btrfs@vger.kernel.org, io-uring@vger.kernel.org, nilay@linux.ibm.com, ritesh.list@gmail.com Subject: Re: [PATCH v4 05/11] block: Add core atomic write support Message-ID: References: <20240219130109.341523-1-john.g.garry@oracle.com> <20240219130109.341523-6-john.g.garry@oracle.com> Precedence: bulk X-Mailing-List: io-uring@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20240219130109.341523-6-john.g.garry@oracle.com> On Mon, Feb 19, 2024 at 01:01:03PM +0000, John Garry wrote: > Add atomic write support as follows: > diff --git a/block/blk-merge.c b/block/blk-merge.c > index 74e9e775f13d..12a75a252ca2 100644 > --- a/block/blk-merge.c > +++ b/block/blk-merge.c > @@ -18,6 +18,42 @@ > #include "blk-rq-qos.h" > #include "blk-throttle.h" > > +static bool rq_straddles_atomic_write_boundary(struct request *rq, > + unsigned int front, > + unsigned int back) > +{ > + unsigned int boundary = queue_atomic_write_boundary_bytes(rq->q); > + unsigned int mask, imask; > + loff_t start, end; > + > + if (!boundary) > + return false; > + > + start = rq->__sector << SECTOR_SHIFT; > + end = start + rq->__data_len; > + > + start -= front; > + end += back; > + > + /* We're longer than the boundary, so must be crossing it */ > + if (end - start > boundary) > + return true; > + > + mask = boundary - 1; > + > + /* start/end are boundary-aligned, so cannot be crossing */ > + if (!(start & mask) || !(end & mask)) > + return false; > + > + imask = ~mask; > + > + /* Top bits are different, so crossed a boundary */ > + if ((start & imask) != (end & imask)) > + return true; > + > + return false; > +} I have no way of verifying this function is doing what it is supposed to because it's function is undocumented. I have no idea what the front/back variables are supposed to represent, and so no clue if they are being applied properly. That said, it's also applying unsigned 32 bit mask variables to signed 64 bit quantities and trying to do things like "high bit changed" checks on the 64 bit variable. This just smells like a future source of "large offsets don't work like we expected!" bugs. > diff --git a/block/blk-settings.c b/block/blk-settings.c > index 06ea91e51b8b..176f26374abc 100644 > --- a/block/blk-settings.c > +++ b/block/blk-settings.c > @@ -59,6 +59,13 @@ void blk_set_default_limits(struct queue_limits *lim) > lim->zoned = false; > lim->zone_write_granularity = 0; > lim->dma_alignment = 511; > + lim->atomic_write_hw_max_sectors = 0; > + lim->atomic_write_max_sectors = 0; > + lim->atomic_write_hw_boundary_sectors = 0; > + lim->atomic_write_hw_unit_min_sectors = 0; > + lim->atomic_write_unit_min_sectors = 0; > + lim->atomic_write_hw_unit_max_sectors = 0; > + lim->atomic_write_unit_max_sectors = 0; > } Seems to me this function would do better to just memset(lim, 0, sizeof(*lim)); and then set all the non-zero fields. > > /** > @@ -101,6 +108,44 @@ void blk_queue_bounce_limit(struct request_queue *q, enum blk_bounce bounce) > } > EXPORT_SYMBOL(blk_queue_bounce_limit); > > + > +/* > + * Returns max guaranteed sectors which we can fit in a bio. For convenience of > + * users, rounddown_pow_of_two() the return value. > + * > + * We always assume that we can fit in at least PAGE_SIZE in a segment, apart > + * from first and last segments. > + */ > +static unsigned int blk_queue_max_guaranteed_bio_sectors( > + struct queue_limits *limits, > + struct request_queue *q) > +{ > + unsigned int max_segments = min(BIO_MAX_VECS, limits->max_segments); > + unsigned int length; > + > + length = min(max_segments, 2) * queue_logical_block_size(q); > + if (max_segments > 2) > + length += (max_segments - 2) * PAGE_SIZE; > + > + return rounddown_pow_of_two(length >> SECTOR_SHIFT); > +} > + > +static void blk_atomic_writes_update_limits(struct request_queue *q) > +{ > + struct queue_limits *limits = &q->limits; > + unsigned int max_hw_sectors = > + rounddown_pow_of_two(limits->max_hw_sectors); > + unsigned int unit_limit = min(max_hw_sectors, > + blk_queue_max_guaranteed_bio_sectors(limits, q)); > + > + limits->atomic_write_max_sectors = > + min(limits->atomic_write_hw_max_sectors, max_hw_sectors); > + limits->atomic_write_unit_min_sectors = > + min(limits->atomic_write_hw_unit_min_sectors, unit_limit); > + limits->atomic_write_unit_max_sectors = > + min(limits->atomic_write_hw_unit_max_sectors, unit_limit); > +} > + > /** > * blk_queue_max_hw_sectors - set max sectors for a request for this queue > * @q: the request queue for the device > @@ -145,6 +190,8 @@ void blk_queue_max_hw_sectors(struct request_queue *q, unsigned int max_hw_secto > limits->logical_block_size >> SECTOR_SHIFT); > limits->max_sectors = max_sectors; > > + blk_atomic_writes_update_limits(q); > + > if (!q->disk) > return; > q->disk->bdi->io_pages = max_sectors >> (PAGE_SHIFT - 9); > @@ -182,6 +229,62 @@ void blk_queue_max_discard_sectors(struct request_queue *q, > } > EXPORT_SYMBOL(blk_queue_max_discard_sectors); > > +/** > + * blk_queue_atomic_write_max_bytes - set max bytes supported by > + * the device for atomic write operations. > + * @q: the request queue for the device > + * @bytes: maximum bytes supported > + */ > +void blk_queue_atomic_write_max_bytes(struct request_queue *q, > + unsigned int bytes) > +{ > + q->limits.atomic_write_hw_max_sectors = bytes >> SECTOR_SHIFT; > + blk_atomic_writes_update_limits(q); > +} > +EXPORT_SYMBOL(blk_queue_atomic_write_max_bytes); Ok, so this can silently set a limit that is different to what the caller asked to have set? How is the caller supposed to find this out if the smaller limit that was set is not compatible with their configuration? i.e. shouldn't this return an error if the requested size cannot be set exactly as specified? Same concern about the other limits that can be trimmed to be smaller than requested. -Dave. -- Dave Chinner david@fromorbit.com