Messages in this thread | | | Date | Thu, 7 Jun 2018 17:26:39 +0200 | From | David Sterba <> | Subject | Re: [PATCH v2] fs: btrfs: Change return type to vm_fault_t |
| |
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.
| |