[PATCH] btrfs: start deprecation of mount option inode_cache

[PATCH] btrfs: start deprecation of mount option inode_cache

[David Sterba]

Estimated time of removal of the functionality is 5.11, the option will
be still parsed but not doing anything.

Reasons for deprecation and removal:

- very poor naming choice of the mount option, it's supposed to cache
  and reuse the inode _numbers_, but it sounds a some generic cache for
  inodes

- the only known usecase where this option would make sense is on a
  32bit architecture where inode numbers in one subvolume would be
  exhausted due to 32bit inode::i_ino

- the cache is stored on disk, consumes space, needs to be loaded and
  written back

- new inode number allocation is slower due to lookups into the cache
  (compared to a simple increment which is the default)

- uses the free-space-cache code that is going to be deprecated as well
  in the future

Known problems:

- since 2011, returning EEXIST when there's not enough space in a page
  to store all checksums, see commit 4b9465cb9e38 ("Btrfs: add mount -o
  inode_cache")

Remaining issues:

- if the option was enabled, new inodes created, the option disabled
  again, the cache is still stored on the devices and there's currently
  no way to remove it

Signed-off-by: David Sterba <dsterba@suse.com>
---
 fs/btrfs/super.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
index 98fe2a634c70..3f1abbeef66c 100644
--- a/fs/btrfs/super.c
+++ b/fs/btrfs/super.c
@@ -827,6 +827,8 @@ int btrfs_parse_options(struct btrfs_fs_info *info, char *options,
 			}
 			break;
 		case Opt_inode_cache:
+			btrfs_warn(info,
+	"the 'inode_cache' option is deprecated and will be stop working in 5.11");
 			btrfs_set_pending_and_info(info, INODE_MAP_CACHE,
 					   "enabling inode map caching");
 			break;
-- 
2.25.0

Re: [PATCH] btrfs: start deprecation of mount option inode_cache

[Qu Wenruo]

[-- Attachment #1.1: Type: text/plain, Size: 2142 bytes --]



On 2020/6/24 上午2:50, David Sterba wrote:
> Estimated time of removal of the functionality is 5.11, the option will
> be still parsed but not doing anything.

Great that we don't need to test for a mount option that no one really uses.

> 
> Reasons for deprecation and removal:
> 
> - very poor naming choice of the mount option, it's supposed to cache
>   and reuse the inode _numbers_, but it sounds a some generic cache for
>   inodes
> 
> - the only known usecase where this option would make sense is on a
>   32bit architecture where inode numbers in one subvolume would be
>   exhausted due to 32bit inode::i_ino
> 
> - the cache is stored on disk, consumes space, needs to be loaded and
>   written back
> 
> - new inode number allocation is slower due to lookups into the cache
>   (compared to a simple increment which is the default)
> 
> - uses the free-space-cache code that is going to be deprecated as well
>   in the future
> 
> Known problems:
> 
> - since 2011, returning EEXIST when there's not enough space in a page
>   to store all checksums, see commit 4b9465cb9e38 ("Btrfs: add mount -o
>   inode_cache")
> 
> Remaining issues:
> 
> - if the option was enabled, new inodes created, the option disabled
>   again, the cache is still stored on the devices and there's currently
>   no way to remove it

What about "btrfs rescue remove-deprecated-feature inode_cache"?
I really don't want kernel to do the hassle.

Thanks,
Qu

> 
> Signed-off-by: David Sterba <dsterba@suse.com>
> ---
>  fs/btrfs/super.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
> index 98fe2a634c70..3f1abbeef66c 100644
> --- a/fs/btrfs/super.c
> +++ b/fs/btrfs/super.c
> @@ -827,6 +827,8 @@ int btrfs_parse_options(struct btrfs_fs_info *info, char *options,
>  			}
>  			break;
>  		case Opt_inode_cache:
> +			btrfs_warn(info,
> +	"the 'inode_cache' option is deprecated and will be stop working in 5.11");
>  			btrfs_set_pending_and_info(info, INODE_MAP_CACHE,
>  					   "enabling inode map caching");
>  			break;
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] btrfs: start deprecation of mount option inode_cache

[Steven Davies]

On 2020-06-23 19:50, David Sterba wrote:
> @@ -827,6 +827,8 @@ int btrfs_parse_options(struct btrfs_fs_info
> *info, char *options,
>  			}
>  			break;
>  		case Opt_inode_cache:
> +			btrfs_warn(info,
> +	"the 'inode_cache' option is deprecated and will be stop working in 
> 5.11");

"will stop working", but perhaps "will have no effect from" might be 
more explicit.

-- 
Steven Davies

Re: [PATCH] btrfs: start deprecation of mount option inode_cache

[David Sterba]

On Wed, Jun 24, 2020 at 08:37:13AM +0100, Steven Davies wrote:
> On 2020-06-23 19:50, David Sterba wrote:
> > @@ -827,6 +827,8 @@ int btrfs_parse_options(struct btrfs_fs_info
> > *info, char *options,
> >  			}
> >  			break;
> >  		case Opt_inode_cache:
> > +			btrfs_warn(info,
> > +	"the 'inode_cache' option is deprecated and will be stop working in 
> > 5.11");
> 
> "will stop working", but perhaps "will have no effect from" might be 
> more explicit.

Thanks, that's better, I'll update the patch.

Re: [PATCH] btrfs: start deprecation of mount option inode_cache

[David Sterba]

On Wed, Jun 24, 2020 at 07:27:46AM +0800, Qu Wenruo wrote:
> > Remaining issues:
> > 
> > - if the option was enabled, new inodes created, the option disabled
> >   again, the cache is still stored on the devices and there's currently
> >   no way to remove it
> 
> What about "btrfs rescue remove-deprecated-feature inode_cache"?
> I really don't want kernel to do the hassle.

Most likely we'll need both, the kernel part is for cases where it's not
so easy to access the filesystem unmounted to do the change. Like a root
partition. How to do that to be least intrusive is the open.

Re: [PATCH] btrfs: start deprecation of mount option inode_cache

[Qu Wenruo]

[-- Attachment #1.1: Type: text/plain, Size: 767 bytes --]



On 2020/6/24 下午7:00, David Sterba wrote:
> On Wed, Jun 24, 2020 at 07:27:46AM +0800, Qu Wenruo wrote:
>>> Remaining issues:
>>>
>>> - if the option was enabled, new inodes created, the option disabled
>>>   again, the cache is still stored on the devices and there's currently
>>>   no way to remove it
>>
>> What about "btrfs rescue remove-deprecated-feature inode_cache"?
>> I really don't want kernel to do the hassle.
> 
> Most likely we'll need both, the kernel part is for cases where it's not
> so easy to access the filesystem unmounted to do the change. Like a root
> partition. How to do that to be least intrusive is the open.
> 
OK. Then what about do it in btrfs_orphan_cleanup()?
It looks like a perfect match.

Thanks,
Qu


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] btrfs: start deprecation of mount option inode_cache

[David Sterba]

On Tue, Jun 23, 2020 at 08:50:32PM +0200, David Sterba wrote:
> Remaining issues:
> 
> - if the option was enabled, new inodes created, the option disabled
>   again, the cache is still stored on the devices and there's currently
>   no way to remove it

The diffstat of the removal is

 b/fs/btrfs/Makefile           |    2 
 b/fs/btrfs/ctree.h            |    5 
 b/fs/btrfs/disk-io.c          |    2 
 b/fs/btrfs/free-space-cache.c |   88 ------
 b/fs/btrfs/inode.c            |   72 ++++-
 b/fs/btrfs/ioctl.c            |    1 
 b/fs/btrfs/relocation.c       |    1 
 b/fs/btrfs/super.c            |   10 
 b/fs/btrfs/transaction.c      |   16 -
 b/fs/btrfs/tree-log.c         |    2 
 fs/btrfs/inode-map.c          |  580 ------------------------------------------
 fs/btrfs/inode-map.h          |   16 -
 12 files changed, 72 insertions(+), 723 deletions(-)

whoever would like to claim it in the 5.11 develoment time will have to
provide fix for the remaining issue(s).