On Mon, Jul 06, 2020 at 10:33:56AM -0400, Josef Bacik wrote: > On 7/6/20 10:05 AM, Qu Wenruo wrote: > >> This only needs to be handled for FLUSH_ALL and FLUSH_STEAL. Anybody > >> doing btrfs_start_transaction() should be able to fail with -EINTR, > >> because they should be close to the syscall path. Balance needs to be > >> fixed to deal with it, and I assume there might be a few other places, > >> but by-in-large none of these places should flip read only. Thanks, > > > > There are already ones existing, for btrfs_drop_snapshot(). > > > > This is mostly caused by btrfs_delalloc_reserve_metadata(), which always > > use FLUSH_ALL unless there is a running trans or its space cache inode. > > > > But still, for a lot of relocation code, we don't really want to bother > > the EINTR and just want uninterruptible at least for now. > > > > Any idea for that? > > Or just rework how we handle errors in a lot of places? > > > > It doesn't look correct for such a low level mechanism to return -EINTR > > while most of callers doesn't really want to bother. > > > > That's the point, most callers shouldn't have to, because most callers shouldn't > be far enough into their operations that -EINTR causes problems. Agreed, that's a sane pattern to follow so we should convert the remaining cases, perhaps also auditing all existing btrfs_start_transaction calls but after a quick look I haven't spotted anything else than the reloc and drop snapshot. > We should probably just change btrfs_drop_snapshot to use join, and then have it > do any space reservation it needs outside of the trans handle. The other option > is a FLUSH_ALL_UNKILLABLE. Thanks, I'd rather not push the killable semantics to the flushing state machine and let it up to the caller context to decide.
