[0/7] Allocate structures on stack instead of kmalloc()

Message

Goldwyn Rodrigues July 27, 2021, 9:17 p.m. UTC

Here are some structs which can be converted to allocation on stack in order
to save on post-checks on kmalloc() and kfree() each of them.

Sizes of the structures are also in the commit message in case you feel they
are too bit to be allocated on stack.

There are two more structs in ioctl.c which can be converted, but
I was not sure of them:

1. In create_snapshot(), pending_snapshot can be converted. pending_snapshot
   is used in the transaction.
2. In btrfs_ioctl_set_received_subvol_32, args64 can be converted, but args32
   cannot. All Pointers associated with memdup_user() can also be converted
   by using copy_from_user() instead. This would include many more structs.

Goldwyn Rodrigues (7):
  btrfs: Allocate walk_control on stack
  btrfs: Allocate file_ra_state on stack
  btrfs: Allocate btrfs_ioctl_get_subvol_info_args on stack
  btrfs: Allocate btrfs_ioctl_balance_args on stack
  btrfs: Allocate btrfs_ioctl_quota_rescan_args on stack
  btrfs: Allocate btrfs_ioctl_defrag_range_args on stack
  btrfs: Alloc backref_ctx on stack

 fs/btrfs/extent-tree.c      |  89 +++++++++++++-----------------
 fs/btrfs/free-space-cache.c |  12 ++---
 fs/btrfs/ioctl.c            | 105 ++++++++++++++----------------------
 fs/btrfs/send.c             |  29 ++++------
 4 files changed, 89 insertions(+), 146 deletions(-)

Comments

David Sterba July 29, 2021, 4:51 p.m. UTC | #1

On Tue, Jul 27, 2021 at 04:17:24PM -0500, Goldwyn Rodrigues wrote:
> Here are some structs which can be converted to allocation on stack in order
> to save on post-checks on kmalloc() and kfree() each of them.
> 
> Sizes of the structures are also in the commit message in case you feel they
> are too bit to be allocated on stack.

Reducing the potential error failures is good and may slightly increase
performance in case the system is low on memory and allocator would have
to do some work.

We must also try to reduce stack usage, but for the ioctls it should be
safe as it's run from the process context and not from a deep call
chain. Where it could be a problem, if the call chaing becomes deep
under btrfs, ie. in any other intermediate layer like NFS, DM, block
layer and drivers.

So, I'd limit the size of on-stack variables to something like 128, that
accounts for a few nested function calls with a few varaibles (eg. 4
functions with 4 pointers each).. This is a normal pattern anywhere
else.

> There are two more structs in ioctl.c which can be converted, but
> I was not sure of them:
> 
> 1. In create_snapshot(), pending_snapshot can be converted. pending_snapshot
>    is used in the transaction.

That one is 144 bytes, arguably still ok.

> 2. In btrfs_ioctl_set_received_subvol_32, args64 can be converted, but args32
>    cannot. All Pointers associated with memdup_user() can also be converted
>    by using copy_from_user() instead. This would include many more structs.

size of btrfs_ioctl_received_subvol_args_32 is 192,
and btrfs_ioctl_received_subvol_args is 200 bytes, both in the same
function.

I'd leave this one as is, it's not something critical where performance
matters.

So, overall I'll apply the series without the two commented.