Re: [PATCH v1 2/2] fs/writeback: skip inodes with potential writeback hang in wait_sb_inodes()

["David Hildenbrand (Red Hat)" <david@kernel.org>]

>>
>> I understand that it might make one of the weird fuse scenarios (buggy
>> fuse server) work again, but it sounds like we are adding more hacks on
>> top of broken semantics. If we want to tackle the writeback problem, we
>> should find a proper way to deal with that for good.
> 
> I agree that this doesn't solve the underlying problem that folios
> belonging to a malfunctioning fuse server may be stuck in writeback
> state forever. To properly and comprehensively address that and the
> other issues (which you alluded to a bit in section 3 below) would I
> think be a much larger effort, but as I understand it, a userspace
> regression needs to be resolved more immediately. 

Right, and that should have be spelled out more clearly in the patch 
description.

The "Currently, fuse is the only filesystem with this flag set. For a
properly functioning fuse server, writeback requests are completed and
there is no issue." part doesn't emphasis that there is no need to wait 
for a different reason: there are no data integrity guarantees with 
fuse, and trying to provide them is currently shaky.

> I wasn't aware that
> if the regression is caused by a faulty userspace program, that rule
> still holds, but I was made aware of that.

Yeah, that is weird though. But I don't understand the details there. 
Hanging the kernel in any case is beyond nasty.

> Even though there are other
> ways that sync could be held up by a faulty/malicious userspace
> program prior to the changes that were added in commit 0c58a97f919c
> ("fuse: remove tmp folio for writebacks and internal rb tree"), I
> think the issue is that that commit gives malfunctioning servers
> another way, which may be a way that some well-intended but buggy
> servers could trigger, which is considered a regression. If it's
> acceptable to delay addressing this until the actual solution that
> addresses the entire problem, then I agree that this patchset is
> unnecessary and we should just wait for the more comprehensive
> solution.
> 
>>
>>
>> (1) AS_WRITEBACK_MAY_HANG semantics
>>
>> As discussed in the past, writeeback of pretty much any filesystem might
>> hang forever on I/O errors.
>>
>> On network filesystems apparently as well fairly easily.
>>
>> It's completely unclear when to set AS_WRITEBACK_MAY_HANG.
>>
>> So as writeback on any filesystem may hang, AS_WRITEBACK_MAY_HANG would
>> theoretically have to be set on any mapping out there.
>>
>> The semantics don't make sense to me, unfortuantely.
> 
> I'm not sure what a better name here would be unfortunately. I
> considered AS_WRITEBACK_UNRELIABLE and AS_WRITEBACK_UNSTABLE but I
> think those run into the same issue where that could technically be
> true of any filesystem (eg the block layer may fail the writeback, so
> it's not completely reliable/stable).

See below, I think this here is really about "no data integrity 
guarantees, so no need to wait even if writeback would be working 
perfectly".

The reproduced hang is rather a symptom of us now trying to achieve data 
integrity when it was never guaranteed.

At least that's my understanding after reading Miklos reply.

> 
>>
>>
>> (2) AS_WRITEBACK_MAY_HANG usage
>>
>> It's unclear in which scenarios we would not want to wait for writeback,
>> and what the effects of that are.
>>
>> For example, wait_sb_inodes() documents "Data integrity sync. Must wait
>> for all pages under writeback, because there may have been pages dirtied
>> before our sync call ...".
>>
>> It's completely unclear why it might be okay to skip that simply because
>> a mapping indicated that waiting for writeback is maybe more sketchy
>> than on other filesystems.
>>
>> But what concerns me more is what we do about other
>> folio_wait_writeback() callers. Throwing in AS_WRITEBACK_MAY_HANG
>> wherever somebody reproduced a hang is not a good approach.
> 
> If I'm recalling this correctly (I'm looking back at this patchset [1]
> to trigger my memory), there were 3 cases where folio_wait_writeback()
> callers run into issues: reclaim, sync, and migration. 

I suspect there are others where we could hang forever, but maybe 
limited to operations where an operation would be stuck forever. Or new 
ones would simply be added in the future.

For example, memory_failure() calls folio_wait_writeback() and I don't 
immediately see why that one would not be able to hit fuse.

So my concern remains. We try to fix the fallout while we really need a 
long term plan of how to deal with that mess.

Sorry that you are the poor soul that opened this can of worms,

[...]

>>
>> Regarding the patch here, is there a good reason why fuse does not have
>> to wait for the "Data integrity sync. Must wait for all pages under
>> writeback ..."?
>>
>> IOW, is the documented "must" not a "must" for fuse? In that case,
> 
> Prior to the changes added in commit 0c58a97f919c ("fuse: remove tmp
> folio for writebacks and internal rb tree"), fuse didn't ensure that
> data was written back for sync. The folio was marked as not under
> writeback anymore, even if it was still under writeback.

Okay, so this really is a fuse special thing.

> 
>> having a flag that states something like that that
>> "AS_NO_WRITEBACK_WAIT_ON_DATA_SYNC" would probable be what we would want
>> to add to avoid waiting for writeback with clear semantics why it is ok
>> in that specific scenario.
> 
> Having a separate AS_NO_WRITEBACK_WAIT_ON_DATA_SYNC mapping flag
> sounds reasonable to me and I agree is more clearer semantically.

Good. Then it's clear that we are not waiting because writeback is 
shaky, but because even if it would be working, because we don't have to 
because there are no such guarantees.

Maybe

AS_NO_DATA_INTEGRITY

or similar would be cleaner, I'll have to leave that to you and Miklos 
to decide what exactly the semantics are that fuse currently doesn't 
provide.


-- 
Cheers

David