On Tue, Oct 25, 2022 at 02:14:49PM +0100, Filipe Manana wrote: > On Tue, Oct 25, 2022 at 2:29 AM Sweet Tea Dorminy > <sweettea-kernel@xxxxxxxxxx> wrote: > > @@ -5523,7 +5560,7 @@ void btrfs_evict_inode(struct inode *inode) > > > > /* > > * Return the key found in the dir entry in the location pointer, fill @type > > - * with BTRFS_FT_*, and return 0. > > + * with BTRFS_FT_*, and return 0. Used only for lookups, not removals. > > This is a bit confusing. What removals? > Isn't it clear the function is used only for lookups? Agreed, update removed. > > > * > > * If no dir entries were found, returns -ENOENT. > > * If found a corrupted location in dir entry, returns -EUCLEAN. > > @@ -5531,18 +5568,27 @@ void btrfs_evict_inode(struct inode *inode) > > @@ -1630,9 +1633,23 @@ static noinline int create_pending_snapshot(struct btrfs_trans_handle *trans, > > ASSERT(pending->root_item); > > new_root_item = pending->root_item; > > > > + /* > > + * Since this is during btrfs_commit_transaction() and more items > > + * joining the transaction at this point would be bad, use NOFS > > + * allocations so that no new writes are kicked off. > > + */ > > This comment makes no sense to me. > > The reason we have to use NOFS it's because when a memory allocation > triggers reclaim it may recurse into the filesystem and > trigger transaction start/join/attach, which would result in a > deadlock (see below why exactly). > > The "more items joining the transaction at this point would be bad" > makes no sense because it's simply not possible. > At this point the transaction is in the state > TRANS_STATE_COMMIT_DOING, so no one can join it and use it for further > modifying the fs - anyone trying to start a new transaction, join or > attach this one will block until the transaction state > becomes >= TRANS_STATE_UNBLOCKED and after that it will have to start > a new transaction (can't reuse the former). I've updated the commit so it says why whe need the NOFS protection similar to what we have elsewhere. There's one GFP_KERNEL allocation in fscrypt_setup_filename so the protection is needed.
|