Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20230806230627.1394689-1-mjguzik@gmail.com> <87o7jidqlg.fsf@email.froward.int.ebiederm.org>
In-Reply-To: <87o7jidqlg.fsf@email.froward.int.ebiederm.org>
From:   Linus Torvalds <torvalds@linux-foundation.org>
Date:   Tue, 8 Aug 2023 09:57:04 -0700
Message-ID: <CAHk-=whk-8Pv5YXH4jNfyAf2xiQCGCUVyBWw71qJEafn4mT6vw@mail.gmail.com>
Subject: Re: [PATCH] fs: use __fput_sync in close(2)
To:     "Eric W. Biederman" <ebiederm@xmission.com>
Cc:     Mateusz Guzik <mjguzik@gmail.com>, viro@zeniv.linux.org.uk,
        linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org,
        oleg@redhat.com, Matthew Wilcox <willy@infradead.org>,
        Christian Brauner <brauner@kernel.org>
Content-Type: multipart/mixed; boundary="000000000000a6196606026c4195"
Precedence: bulk

--000000000000a6196606026c4195
Content-Type: text/plain; charset="UTF-8"

On Mon, 7 Aug 2023 at 22:57, Eric W. Biederman <ebiederm@xmission.com> wrote:
>
> Taking a quick look at the history it appears that fput was always
> synchronous [..]

Indeed. Synchronous used to be the only case.

The reason it's async now is because several drivers etc do the final
close from nasty contexts, so 'fput()' needed to be async for the
general case.

> All 3 issues taken together says that a synchronous fput is a
> loaded foot gun that must be used very carefully.   That said
> close(2) does seem to be a reliably safe place to be synchronous.

Yes.

That said, I detest Mateusz' patch. I hate these kinds of "do
different things based on flags" interfaces. Particularly when it
spreads out like this.

So I do like having close() be synchronous, because we actually do
have correctness issues wrt the close having completed properly by the
time we return to user space, so we have that "task_work_add()" there
that will force the synchronization anyway before we return.

So the system call case is indeed a special case. Arguably
close_range() could be too, but honestly, once you start doing ranges
of file descriptors, you are (a) doint something fairly unusual, and
(b) the "queue them up on the task work" might actually be a *good*
thing.

It's definitely not a good thing for the single-fd-close case, though.

But even if we want to do this - and I have absolutely no objections
to it conceptually as per above - we need to be a lot more surgical
about it, and not pass stupid flags around.

Here's a TOTALLY UNTESTED(!) patch that I think effectively does what
Mateusz wants done, but does it all within just fs/open.c and only for
the obvious context of the close() system call itself.

All it needs is to just split out the "flush" part from filp_close(),
and we already had all the other infrastructure for this operation.

Mateusz, two questions:

 (a) does this patch work for you?

 (b) do you have numbers for this all?

and if it all looks good I have no problems with this kind of much
more targeted and obvious patch.

Again: TOTALLY UNTESTED. It looks completely obvious, but mistakes happen.

             Linus

--000000000000a6196606026c4195
Content-Type: text/x-patch; charset="US-ASCII"; name="patch.diff"
Content-Disposition: attachment; filename="patch.diff"
Content-Transfer-Encoding: base64
Content-ID: <f_ll2jn96d0>
X-Attachment-Id: f_ll2jn96d0

IGZzL29wZW4uYyB8IDIyICsrKysrKysrKysrKysrKysrKystLS0KIDEgZmlsZSBjaGFuZ2VkLCAx
OSBpbnNlcnRpb25zKCspLCAzIGRlbGV0aW9ucygtKQoKZGlmZiAtLWdpdCBhL2ZzL29wZW4uYyBi
L2ZzL29wZW4uYwppbmRleCBlNmVhZDBmMTk5NjQuLjVmZTQyN2U4Njg1YyAxMDA2NDQKLS0tIGEv
ZnMvb3Blbi5jCisrKyBiL2ZzL29wZW4uYwpAQCAtMTUwMyw3ICsxNTAzLDcgQEAgU1lTQ0FMTF9E
RUZJTkUyKGNyZWF0LCBjb25zdCBjaGFyIF9fdXNlciAqLCBwYXRobmFtZSwgdW1vZGVfdCwgbW9k
ZSkKICAqICJpZCIgaXMgdGhlIFBPU0lYIHRocmVhZCBJRC4gV2UgdXNlIHRoZQogICogZmlsZXMg
cG9pbnRlciBmb3IgdGhpcy4uCiAgKi8KLWludCBmaWxwX2Nsb3NlKHN0cnVjdCBmaWxlICpmaWxw
LCBmbF9vd25lcl90IGlkKQorc3RhdGljIGludCBmaWxwX2ZsdXNoKHN0cnVjdCBmaWxlICpmaWxw
LCBmbF9vd25lcl90IGlkKQogewogCWludCByZXR2YWwgPSAwOwogCkBAIC0xNTIwLDEwICsxNTIw
LDE1IEBAIGludCBmaWxwX2Nsb3NlKHN0cnVjdCBmaWxlICpmaWxwLCBmbF9vd25lcl90IGlkKQog
CQlkbm90aWZ5X2ZsdXNoKGZpbHAsIGlkKTsKIAkJbG9ja3NfcmVtb3ZlX3Bvc2l4KGZpbHAsIGlk
KTsKIAl9Ci0JZnB1dChmaWxwKTsKIAlyZXR1cm4gcmV0dmFsOwogfQogCitpbnQgZmlscF9jbG9z
ZShzdHJ1Y3QgZmlsZSAqZmlscCwgZmxfb3duZXJfdCBpZCkKK3sKKwlpbnQgcmV0dmFsID0gZmls
cF9mbHVzaChmaWxwLCBpZCk7CisJZnB1dChmaWxwKTsKKwlyZXR1cm4gcmV0dmFsOworfQogRVhQ
T1JUX1NZTUJPTChmaWxwX2Nsb3NlKTsKIAogLyoKQEAgLTE1MzMsNyArMTUzOCwxOCBAQCBFWFBP
UlRfU1lNQk9MKGZpbHBfY2xvc2UpOwogICovCiBTWVNDQUxMX0RFRklORTEoY2xvc2UsIHVuc2ln
bmVkIGludCwgZmQpCiB7Ci0JaW50IHJldHZhbCA9IGNsb3NlX2ZkKGZkKTsKKwlzdHJ1Y3QgZmls
ZSAqZmlsZSA9IGNsb3NlX2ZkX2dldF9maWxlKGZkKTsKKwlpbnQgcmV0dmFsOworCisJaWYgKCFm
aWxlKQorCQlyZXR1cm4gLUVCQURGOworCXJldHZhbCA9IGZpbHBfZmx1c2goZmlsZSwgY3VycmVu
dC0+ZmlsZXMpOworCisJLyoKKwkgKiBXZSdyZSByZXR1cm5pbmcgdG8gdXNlciBzcGFjZS4gRG9u
J3QgYm90aGVyCisJICogd2l0aCBhbnkgZGVsYXllZCBmcHV0KCkgY2FzZXMuCisJICovCisJX19m
cHV0X3N5bmMoZmlsZSk7CiAKIAkvKiBjYW4ndCByZXN0YXJ0IGNsb3NlIHN5c2NhbGwgYmVjYXVz
ZSBmaWxlIHRhYmxlIGVudHJ5IHdhcyBjbGVhcmVkICovCiAJaWYgKHVubGlrZWx5KHJldHZhbCA9
PSAtRVJFU1RBUlRTWVMgfHwK
--000000000000a6196606026c4195--