Re: [PATCH v2] fs: btrfs: Change return type to vm_fault_t

From: David Sterba
Date: Thu Jun 07 2018 - 11:29:33 EST


On Wed, Jun 06, 2018 at 10:50:49AM -0700, Matthew Wilcox wrote:
> On Wed, Jun 06, 2018 at 05:53:47PM +0200, David Sterba wrote:
> > On Wed, Jun 06, 2018 at 07:54:44PM +0530, Souptick Joarder wrote:
> > > @@ -9009,7 +9007,7 @@ int btrfs_page_mkwrite(struct vm_fault *vmf)
> > > unlock_extent_cached(io_tree, page_start, page_end, &cached_state);
> > >
> > > out_unlock:
> > > - if (!ret) {
> > > + if (!ret2) {
> > > btrfs_delalloc_release_extents(BTRFS_I(inode), PAGE_SIZE, true);
> > > sb_end_pagefault(inode->i_sb);
> > > extent_changeset_free(data_reserved);
> >
> > 9013 return VM_FAULT_LOCKED;
> > 9014 }
> > 9015 unlock_page(page);
> > 9016 out:
> > 9017 btrfs_delalloc_release_extents(BTRFS_I(inode), PAGE_SIZE, (ret != 0));
> > 9018 btrfs_delalloc_release_space(inode, data_reserved, page_start,
> > 9019 reserved_space, (ret != 0));
> >
> > I've noticed that there's 'ret' used on lines 9017 and 19, comparing to
> > a raw number. Is this going to be ok once vm_fault_t is it's own type?
> >
> > There's no corresponding define for 0 among the VM_FAULT_* values, I'd
> > expect 0 to work interchangeably, similar to the blk_status_t type:
> >
> > https://elixir.bootlin.com/linux/latest/source/include/linux/blk_types.h#L30
> >
> > #define BLK_STS_OK 0
> > #define BLK_STS_NOTSUPP ((__force blk_status_t)1)
> > #define BLK_STS_TIMEOUT ((__force blk_status_t)2)
> > #define BLK_STS_NOSPC ((__force blk_status_t)3)
> > ...
> >
> > Your patch is otherwise ok, I'm just curious if this is something to
> > watch for once vmfault type is switched.
>
> Yes, sparse treats 0 specially for these kinds of types. It goes back to
> the original use of __bitwise to mean "this is a special-endian type",
> but it's also meaningful for types which aren't _numbers_ so much as a
> collection of bits.

Ok, thanks.

> By the way, do you really think it improves this function to use 'ret' and
> 'ret2' like this? It's your code, and you're entitled to adopt whatever
> stylistic preferences you like, but I personally find it easier to read
> with 'err' being an errno and 'ret' being the vm_fault_t.

The ret/err pattern caused some confusion so we're going to unify that a
bit and use 'ret' for the function scope return.