From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by smtp.lore.kernel.org (Postfix) with ESMTP id 404AFEB64DE for ; Tue, 10 Sep 2024 10:58:02 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 9EC6E8D005A; Tue, 10 Sep 2024 06:58:01 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 99BB58D0056; Tue, 10 Sep 2024 06:58:01 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 88BAD8D005A; Tue, 10 Sep 2024 06:58:01 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0016.hostedemail.com [216.40.44.16]) by kanga.kvack.org (Postfix) with ESMTP id 6CBF38D0056 for ; Tue, 10 Sep 2024 06:58:01 -0400 (EDT) Received: from smtpin26.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay09.hostedemail.com (Postfix) with ESMTP id 1380B814AF for ; Tue, 10 Sep 2024 10:58:01 +0000 (UTC) X-FDA: 82548528762.26.32733FD Received: from mail-ej1-f53.google.com (mail-ej1-f53.google.com [209.85.218.53]) by imf13.hostedemail.com (Postfix) with ESMTP id 159B720008 for ; Tue, 10 Sep 2024 10:57:58 +0000 (UTC) Authentication-Results: imf13.hostedemail.com; dkim=pass header.d=gmail.com header.s=20230601 header.b=ekKl63QF; spf=pass (imf13.hostedemail.com: domain of asml.silence@gmail.com designates 209.85.218.53 as permitted sender) smtp.mailfrom=asml.silence@gmail.com; dmarc=pass (policy=none) header.from=gmail.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1725965827; a=rsa-sha256; cv=none; b=xXza1yEw3UgQb5Bz4XWHqH2hIwr+XLqFroctk/evlzf8MWjxnCrVXZBhcEH5agvQzz2seU AmlQ4dr52znnHGcb4dcJvAeBZ75wNbTsDeetejfqo5PJ/32Figk993FeeZyfflIYxeuHOa euQK8wSwYw9BpuDRW9WHk/r1PuBY2Gg= ARC-Authentication-Results: i=1; imf13.hostedemail.com; dkim=pass header.d=gmail.com header.s=20230601 header.b=ekKl63QF; spf=pass (imf13.hostedemail.com: domain of asml.silence@gmail.com designates 209.85.218.53 as permitted sender) smtp.mailfrom=asml.silence@gmail.com; dmarc=pass (policy=none) header.from=gmail.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1725965827; h=from:from:sender:reply-to:subject:subject:date:date: message-id:message-id:to:to:cc:cc:mime-version:mime-version: content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references:dkim-signature; bh=SJslusrrfXDGSsSDYUzneHTVhmBl5GEMcjjDhX1iRYg=; b=WOR5qHK8xQfHubfB0nDnZNr8wVkYDSNlrWgZPe7eSsQB6AjKHuXm6D7ia9i5vGJdod7BP3 Qt51Vd/Gn/NnpT398OtxsnuRuLLpDkjthIOI4ydsssp8lbnETOXV3OgJR6u38HX4/EG/MR PpN+5FU0cyp9ac74uU5g2Dws2r/Cy0U= Received: by mail-ej1-f53.google.com with SMTP id a640c23a62f3a-a8d6d0fe021so204706666b.1 for ; Tue, 10 Sep 2024 03:57:58 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1725965877; x=1726570677; darn=kvack.org; h=content-transfer-encoding:in-reply-to:from:content-language :references:cc:to:subject:user-agent:mime-version:date:message-id :from:to:cc:subject:date:message-id:reply-to; bh=SJslusrrfXDGSsSDYUzneHTVhmBl5GEMcjjDhX1iRYg=; b=ekKl63QFVx/FWXhk6PerEmhvN5HBwGd6rmtQYDAv4bBLQ3PUnyR9XCskd4Uom5Vxm1 5ynLo8y6Y2lBle0aMzO+XKL2z8nu5HSOsm0XSRZdTNFwuj32rMqR89A2sTU7N6xN2Hy9 obsGBdCuJZW3csEhZs34NQo1YkXWgQak3UoWLqmUVdKSskPLC7OlQZREpHSyj4lWOQ7N wYd6cGeEBzgbXDSoTzr1LAB7JwhFg05HAxFNKCslVlvp8oNXXmtPqicGLBal2TkRszwI 7A2c7YAplIl9QZ2+B/QRPJwh62Xn/ZcEb3uaWGAUzJbQEHSu0Ug4YXyRjvrEyBXJ6VEx mTvw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1725965877; x=1726570677; h=content-transfer-encoding:in-reply-to:from:content-language :references:cc:to:subject:user-agent:mime-version:date:message-id :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=SJslusrrfXDGSsSDYUzneHTVhmBl5GEMcjjDhX1iRYg=; b=lSzg+VQsCaQirj79ymLGlmksPJtaxRnYSJq28nKh2+WoRKGyeopCUTXxkD/a7ZnFNG syRklr0moxpLnJcOQY4an8zY7oemSQWabbmxxIT6K2s56oh9zv7ckEGcWm5cwutYW9xm HwC+96NYKcPy0gctpE/7jZ+Y5tAD6LByinqLw6UriNBkkg3qDC30wwGC91qWYHugoY7G ivOL827ypzRPDMKiZhkkl6/j+4/risSatJjQJrHtaxxvYXVPccAJtp7/8aJzaYN5kagG VHCx6ggOhD7tlPqmXdgKYr5zwTPeAgrrYImD+f+hchTxVKsCQrvofhP8NlcbnnGtI6eT LtsA== X-Forwarded-Encrypted: i=1; AJvYcCUxquz55q9JrSuHFBshqwWpVPxor4LGh6gBLj05VmpuZtTJc/QSLQxTzABVwKTYg5mBNXszd/D7kg==@kvack.org X-Gm-Message-State: AOJu0Yw1nJm0dtz0dewiUDI7rWd1FiyZWT9+CFBD6osd7h/1NUMbiUDS ksEZbgDj+D+L61JKd+Ieb92qnpR+i7q+WKBqFSmgO3pfRUY2i91s X-Google-Smtp-Source: AGHT+IHJrz5lkl252drUfjQ7T3c25l0GDv7hN76cAIIs9WY19rUFlfCKvwm1+Pw85rtaKK0gycWHsw== X-Received: by 2002:a17:907:c29:b0:a8f:f799:e7d1 with SMTP id a640c23a62f3a-a8ffab7e36amr36896466b.38.1725965876614; Tue, 10 Sep 2024 03:57:56 -0700 (PDT) Received: from [192.168.42.232] ([163.114.131.193]) by smtp.gmail.com with ESMTPSA id a640c23a62f3a-a8d258354d1sm467851266b.13.2024.09.10.03.57.55 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Tue, 10 Sep 2024 03:57:56 -0700 (PDT) Message-ID: <430ca5b3-6ee1-463b-9e4e-5d0b934578cc@gmail.com> Date: Tue, 10 Sep 2024 11:58:23 +0100 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH v4 5/8] block: implement async discard as io_uring cmd To: Christoph Hellwig Cc: io-uring@vger.kernel.org, Jens Axboe , Conrad Meyer , linux-block@vger.kernel.org, linux-mm@kvack.org References: <7fc0a61ae29190a42e958eddfefd6d44cdf372ad.1725621577.git.asml.silence@gmail.com> Content-Language: en-US From: Pavel Begunkov In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-Stat-Signature: cj4g8bynde4t8g46b6soaqk765x7knje X-Rspamd-Queue-Id: 159B720008 X-Rspam-User: X-Rspamd-Server: rspam10 X-HE-Tag: 1725965878-704595 X-HE-Meta: U2FsdGVkX1/sOfhAuiA05n8vGopmyD4PrqJuuFZ54H9gwUb+wxjDI8KAcb2F/oxc1silMoJ5yUO82a1yuafwVFyFIP6qtfATK/7nqJMli48UHYcW40j7NDei08UTpMk4/+92RmvZq0L+ome0Grs+O9oUzdYE8QWZgFRgZNp5JuFpamnAkWwHBgieynn0eO7WtuGl5XTrnhamW+UGuxbK3OeCqh2NCIDH1TyN/P0zSS9hM+XRNTiJrUNjAXuEgJ/2d3O/RZPRXv6XN2LILkk45wCryJ1D7m6eJpLFoyhunakH5nxzfur38zFZntaQHcnNk/yrvgM7nSnjZm2LfXR88I09V0RX8xbVg2/9XFWdfJTUcn1Mwfdm5IfazvYr79rce0jkW8hNgPFXVgQczUbEFgZpAusovHYi1RnU1COP3yuUQHIf+2VIL7e0FptLU0f5e0dRY5w6n7BGLpUHK8hXuAUIdEqa8UAwG4JDmGyKyu2ExfqIDYf9qm1ExKlxG2SYUfBUrOEK4euxKlr+Lr7wlVq2LSF0ofQMXltBEH9bKQZy5snJrx5Ud+Xm/F/ZoDsQKWRQDZNI4K32cKbZ5YrNy+NES15GqEc/78yBJBnH5KG8ZT7pYUAoIreFV8eZIrnQGZeTcVpc2vS3Qw/M4fMSCEd1zQfPMqjy39agYUbFvP5fSJR95dCSAuvs3ru9nVIB5fQZu9tR59VWrE+fTerc4HQef7z3k5Pr607uN3zFBqJkstnnaICLXp08aC4kRrlFafTOpVc46cBwmtJPa5VYzauC1sx7iEOXO+HfpsX6U7Xn3EF+HDSZ6efTz77D94IR4x3dpLpvKTnUBRtNzq0n5yx9fFJEKltxosV41ZMPH1YtNJyT0nTahh86H3+BI5nMEjjyQ0kx5qHhF+Jj5pqor1eHh6hUS5BxmiEey4ba26rvG2kdCBvqbavXCprqtSJHFPlp0ax8jRG20jC+o0r SKcuGabv 8oa/rY49TaXsojpRskbnJhr3F8scqKeQmOvA0PbGK2dnkCI2Bp3izjHZBUpKPksmGxWMzvo90d1HM7cHaCnAOFun52ouKezZC8OVRzfix6MUcqEkxUFZx/U9rhjxviWr5OlqwrSoDj2C78/Dkr3HEPZf04vkdx4IkXmGsnmn0byrlHiVWrz3r6IY4Kzg0NkDdG37T4w9Iq/ZhG3zpQToo9gEycqSBsfPRYBoFrzS6Wb6kZ5U/tm2E2e8octYdOVJdhc9/gQI81QyfU3km6vYgew3FjsSm11vqQH0ZsKoFMg0VCzoLulx0Ee7UeuX5Dra0HnTCy12LhoOuFeb+HXR0cHsI8HRVO9ImAgzC4oVcsK34PO++l6m1+/tvGM07T3FUlm6zGpWKS/3aRb/k4e7ToCz4rzExVZp6r9/tlzvh7BIdby5KUJnP+H+TNO558HipNyQ6LoLuEAIwO2MnCRe56RFqWxXVsvNEUjnV X-Bogosity: Ham, tests=bogofilter, spamicity=0.000984, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: List-Subscribe: List-Unsubscribe: On 9/10/24 09:01, Christoph Hellwig wrote: >> + sector_t sector = start >> SECTOR_SHIFT; >> + sector_t nr_sects = len >> SECTOR_SHIFT; >> + struct bio *prev = NULL, *bio; >> + int err; >> + >> + if (!bdev_max_discard_sectors(bdev)) >> + return -EOPNOTSUPP; >> + >> + if (!(file_to_blk_mode(cmd->file) & BLK_OPEN_WRITE)) >> + return -EBADF; >> + if (bdev_read_only(bdev)) >> + return -EPERM; >> + err = blk_validate_byte_range(bdev, start, len); >> + if (err) >> + return err; > > Based on the above this function is misnamed, as it validates sector_t > range and not a byte range. Start and len here are in bytes. What do you mean? >> + if (nowait && nr_sects > bio_discard_limit(bdev, sector)) >> + return -EAGAIN; >> + >> + err = filemap_invalidate_pages(bdev->bd_mapping, start, >> + start + len - 1, nowait); >> + if (err) >> + return err; >> + >> + while ((bio = blk_alloc_discard_bio(bdev, §or, &nr_sects, gfp))) { >> + if (nowait) >> + bio->bi_opf |= REQ_NOWAIT; >> + prev = bio_chain_and_submit(prev, bio); >> + } >> + if (!prev) >> + return -EAGAIN; > > If a user changes the max_discard value between the check above and > the loop here this is racy. If the driver randomly changes it, it's racy either way. What do you want to protect against? >> +sector_t bio_discard_limit(struct block_device *bdev, sector_t sector); > > And to be honest, I'd really prefer to not have bio_discard_limit > exposed. Certainly not outside a header private to block/. Which is the other reason why first versions were putting down a bio seeing that there is more to be done for nowait, which you didn't like. I can return back to it or narrow the scopre. >> + >> #endif /* __LINUX_BIO_H */ >> diff --git a/include/uapi/linux/fs.h b/include/uapi/linux/fs.h >> index 753971770733..7ea41ca97158 100644 >> --- a/include/uapi/linux/fs.h >> +++ b/include/uapi/linux/fs.h >> @@ -208,6 +208,8 @@ struct fsxattr { >> * (see uapi/linux/blkzoned.h) >> */ >> >> +#define BLOCK_URING_CMD_DISCARD _IO(0x12,137) > > Whitespace after the comma please. That appears to be the "code style" of all BLK ioctls. > Also why start at 137? A comment > would generally be pretty useful as well. There is a comment, 2 lines above the new define. /* * A jump here: 130-136 are reserved for zoned block devices * (see uapi/linux/blkzoned.h) */ Is that your concern? > Also can we have a include/uapi/linux/blkdev.h for this instead of > bloating fs.h that gets included just about everywhere? I don't think it belongs to this series. Regardless, how do you see it? The new file can have just those several new definitions, in fs.h we'd have another comment why there is another empty range, but I don't think it's worth it at all. Another option is to move there everything block related, and make fs.h include blkdev.h, which can always be done on top. -- Pavel Begunkov