Re: [PATCH v6 03/24] erofs: add super block operations
On Fri, Aug 30, 2019 at 8:16 PM Gao Xiang wrote: > > Hi Christoph, > > On Fri, Aug 30, 2019 at 09:39:10AM -0700, Christoph Hellwig wrote: > > On Thu, Aug 29, 2019 at 06:50:48PM +0800, Gao Xiang wrote: > > > > Please use an erofs_ prefix for all your functions. > > > > > > It is already a static function, I have no idea what is wrong here. > > > > Which part of all wasn't clear? Have you looked at the prefixes for > > most functions in the various other big filesystems? > > I will add erofs prefix to free_inode as you said. > > At least, all non-prefix functions in erofs are all static functions, > it won't pollute namespace... I will add "erofs_" to other meaningful > callbacks...And as you can see... > > cifs/cifsfs.c > 1303:cifs_init_inodecache(void) > 1509: rc = cifs_init_inodecache(); > > hpfs/super.c > 254:static int init_inodecache(void) > 771:int err = init_inodecache(); > > minix/inode.c > 84:static int __init init_inodecache(void) > 665:int err = init_inodecache(); > Hi Gao, "They did it first" is never a good reply for code review comments. Nobody cares if you copy&paste code with init_inodecache(). I understand why you thought static function names do not pollute the (linker) namespace, but they do pollute the global namespace. free_inode() as a local function name is one of the worst examples for VFS namespace pollution. VFS code uses function names like those a lot in the global namespace, e.g.: clear_inode(),new_inode(). For example from recent history of namespace collision caused by your line of thinking, see: e6fd2093a85d md: namespace private helper names Besides, you really have nothing to loose from prefixing everything with erofs_, do you? It's better for review, for debugging... Thanks, Amir. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v3 12/24] erofs: introduce tagged pointer
On Mon, Jul 22, 2019 at 5:54 AM Gao Xiang wrote: > > Currently kernel has scattered tagged pointer usages > hacked by hand in plain code, without a unique and > portable functionset to highlight the tagged pointer > itself and wrap these hacked code in order to clean up > all over meaningless magic masks. > > This patch introduces simple generic methods to fold > tags into a pointer integer. Currently it supports > the last n bits of the pointer for tags, which can be > selected by users. > > In addition, it will also be used for the upcoming EROFS > filesystem, which heavily uses tagged pointer pproach > to reduce extra memory allocation. > > Link: https://en.wikipedia.org/wiki/Tagged_pointer Well, it won't do much good for other kernel users in fs/erofs/ ;-) I think now would be a right time to promote this facility to include/linux as you initially proposed. I don't recall you got any objections. No ACKs either, but I think that was the good kind of silence (?) You might want to post the __fdget conversion patch [1] as a bonus patch on top of your series. Thanks, Amir. [1] https://lore.kernel.org/linux-fsdevel/1530543233-65279-2-git-send-email-gaoxian...@huawei.com/ ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v3 12/24] erofs: introduce tagged pointer
On Mon, Jul 22, 2019 at 8:02 AM Gao Xiang wrote:
>
> Hi Amir,
>
> On 2019/7/22 12:39, Amir Goldstein wrote:
> > On Mon, Jul 22, 2019 at 5:54 AM Gao Xiang wrote:
> >>
> >> Currently kernel has scattered tagged pointer usages
> >> hacked by hand in plain code, without a unique and
> >> portable functionset to highlight the tagged pointer
> >> itself and wrap these hacked code in order to clean up
> >> all over meaningless magic masks.
> >>
> >> This patch introduces simple generic methods to fold
> >> tags into a pointer integer. Currently it supports
> >> the last n bits of the pointer for tags, which can be
> >> selected by users.
> >>
> >> In addition, it will also be used for the upcoming EROFS
> >> filesystem, which heavily uses tagged pointer pproach
> >> to reduce extra memory allocation.
> >>
> >> Link: https://en.wikipedia.org/wiki/Tagged_pointer
> >
> > Well, it won't do much good for other kernel users in fs/erofs/ ;-)
>
> Thanks for your reply and interest in this patch :)
>
> Sigh... since I'm not sure kernel folks could have some interests in that
> stuffs.
>
> Actually at the time once I coded EROFS I found tagged pointer had 2 main
> advantages:
> 1) it saves an extra field;
> 2) it can keep the whole stuff atomicly...
> And I observed the current kernel uses tagged pointer all around but w/o a
> proper wrapper...
> and EROFS heavily uses tagged pointer... So I made a simple tagged pointer
> wrapper
> to avoid meaningless magic masks and type casts in the code...
>
> >
> > I think now would be a right time to promote this facility to
> > include/linux as you initially proposed.
> > I don't recall you got any objections. No ACKs either, but I think
> > that was the good kind of silence (?)
>
> Yes, no NAK no ACK...(it seems the ordinary state for all EROFS stuffs... :'(
> sigh...)
> Therefore I decided to leave it in fs/erofs/ in this series...
>
> >
> > You might want to post the __fdget conversion patch [1] as a
> > bonus patch on top of your series.
>
> I am not sure if another potential users could be quite happy with my
> ("sane?" or not)
> implementation...
Well, let's ask potential users then.
CC kernel/trace maintainers for RB_PAGE_HEAD/RB_PAGE_UPDATE
and kernel/locking maintainers for RT_MUTEX_HAS_WAITERS
> (Is there some use scenerios in overlayfs and fanotify?...)
We had one in overlayfs once. It is gone now.
>
> and I'm not sure Al could accept __fdget conversion (I just wanted to give a
> example then...)
>
> Therefore, I tend to keep silence and just promote EROFS... some better
> ideas?...
>
Writing example conversion patches to demonstrate cleaner code
and perhaps reduce LOC seems the best way.
Also pointing out that fixing potential bugs in one implementation is preferred
to having to patch all copied implementations.
I wonder if tagptr_unfold_tags() doesn't need READ_ONCE() as per:
1be5d4fa0af3 locking/rtmutex: Use READ_ONCE() in rt_mutex_owner()
rb_list_head() doesn't have READ_ONCE()
Nor does hlist_bl_first() and BPF_MAP_PTR().
Are those all safe due to safe call sites? or potentially broken?
Thanks,
Amir.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: exfat: add STAGING prefix to config names
On Fri, Jan 3, 2020 at 3:17 AM Namjae Jeon wrote:
>
> Add STAGING prefix to config names to avoid collsion with fs/exfat config.
>
> Signed-off-by: Namjae Jeon
> ---
> drivers/staging/Makefile | 2 +-
> drivers/staging/exfat/Kconfig| 26 +++
> drivers/staging/exfat/Makefile | 2 +-
> drivers/staging/exfat/exfat.h| 14
> drivers/staging/exfat/exfat_blkdev.c | 12 +++
> drivers/staging/exfat/exfat_core.c | 8 ++---
> drivers/staging/exfat/exfat_super.c | 50 ++--
> 7 files changed, 57 insertions(+), 57 deletions(-)
>
> diff --git a/drivers/staging/Makefile b/drivers/staging/Makefile
> index 463aef6a18ef..fdd03fd6e704 100644
> --- a/drivers/staging/Makefile
> +++ b/drivers/staging/Makefile
> @@ -48,7 +48,7 @@ obj-$(CONFIG_FIELDBUS_DEV) += fieldbus/
> obj-$(CONFIG_KPC2000) += kpc2000/
> obj-$(CONFIG_UWB) += uwb/
> obj-$(CONFIG_USB_WUSB) += wusbcore/
> -obj-$(CONFIG_EXFAT_FS) += exfat/
> +obj-$(CONFIG_STAGING_EXFAT_FS) += exfat/
> obj-$(CONFIG_QLGE) += qlge/
> obj-$(CONFIG_NET_VENDOR_HP)+= hp/
> obj-$(CONFIG_WFX) += wfx/
> diff --git a/drivers/staging/exfat/Kconfig b/drivers/staging/exfat/Kconfig
> index 0130019cbec2..292a19dfcaf5 100644
> --- a/drivers/staging/exfat/Kconfig
> +++ b/drivers/staging/exfat/Kconfig
> @@ -1,41 +1,41 @@
> # SPDX-License-Identifier: GPL-2.0
> -config EXFAT_FS
> +config STAGING_EXFAT_FS
> tristate "exFAT fs support"
> depends on BLOCK
> select NLS
> help
> This adds support for the exFAT file system.
>
> -config EXFAT_DISCARD
> +config STAGING_EXFAT_DISCARD
> bool "enable discard support"
> - depends on EXFAT_FS
> + depends on STAGING_EXFAT_FS
> default y
>
> -config EXFAT_DELAYED_SYNC
> +config STAGING_EXFAT_DELAYED_SYNC
> bool "enable delayed sync"
> - depends on EXFAT_FS
> + depends on STAGING_EXFAT_FS
> default n
>
> -config EXFAT_KERNEL_DEBUG
> +config STAGING_EXFAT_KERNEL_DEBUG
> bool "enable kernel debug features via ioctl"
> - depends on EXFAT_FS
> + depends on STAGING_EXFAT_FS
> default n
>
> -config EXFAT_DEBUG_MSG
> +config STAGING_EXFAT_DEBUG_MSG
> bool "print debug messages"
> - depends on EXFAT_FS
> + depends on STAGING_EXFAT_FS
> default n
>
> -config EXFAT_DEFAULT_CODEPAGE
> +config STAGING_EXFAT_DEFAULT_CODEPAGE
> int "Default codepage for exFAT"
> default 437
> - depends on EXFAT_FS
> + depends on STAGING_EXFAT_FS
> help
> This option should be set to the codepage of your exFAT filesystems.
>
> -config EXFAT_DEFAULT_IOCHARSET
> +config STAGING_EXFAT_DEFAULT_IOCHARSET
> string "Default iocharset for exFAT"
> default "utf8"
> - depends on EXFAT_FS
> + depends on STAGING_EXFAT_FS
> help
> Set this to the default input/output character set you'd like exFAT
> to use.
> diff --git a/drivers/staging/exfat/Makefile b/drivers/staging/exfat/Makefile
> index 6c90aec83feb..057556eeca0c 100644
> --- a/drivers/staging/exfat/Makefile
> +++ b/drivers/staging/exfat/Makefile
> @@ -1,6 +1,6 @@
> # SPDX-License-Identifier: GPL-2.0-or-later
>
> -obj-$(CONFIG_EXFAT_FS) += exfat.o
> +obj-$(CONFIG_STAGING_EXFAT_FS) += exfat.o
>
> exfat-y := exfat_core.o\
> exfat_super.o \
> diff --git a/drivers/staging/exfat/exfat.h b/drivers/staging/exfat/exfat.h
> index 51c665a924b7..3865c17027ce 100644
> --- a/drivers/staging/exfat/exfat.h
> +++ b/drivers/staging/exfat/exfat.h
> @@ -9,7 +9,7 @@
> #include
> #include
>
> -#ifdef CONFIG_EXFAT_KERNEL_DEBUG
> +#ifdef CONFIG_STAGING_EXFAT_KERNEL_DEBUG
>/* For Debugging Purpose */
> /* IOCTL code 'f' used by
> * - file systems typically #0~0x1F
> @@ -22,9 +22,9 @@
>
> #define EXFAT_DEBUGFLAGS_INVALID_UMOUNT0x01
> #define EXFAT_DEBUGFLAGS_ERROR_RW 0x02
> -#endif /* CONFIG_EXFAT_KERNEL_DEBUG */
> +#endif /* CONFIG_STAGING_EXFAT_KERNEL_DEBUG */
>
> -#ifdef CONFIG_EXFAT_DEBUG_MSG
> +#ifdef CONFIG_STAGING_EXFAT_DEBUG_MSG
> #define DEBUG 1
> #else
> #undef DEBUG
> @@ -661,10 +661,10 @@ struct exfat_mount_options {
>
> /* on error: continue, panic, remount-ro */
> unsigned char errors;
> -#ifdef CONFIG_EXFAT_DISCARD
> +#ifdef CONFIG_STAGING_EXFAT_DISCARD
> /* flag on if -o dicard specified and device support discard() */
> unsigned char discard;
> -#endif /* CONFIG_EXFAT_DISCARD */
> +#endif /* CONFIG_STAGING_EXFAT_DISCARD */
> };
>
> #define EXFAT_HASH_BITS8
> @@ -700,9 +700,9 @@ struct exfat_sb_info {
>
> spinlock_t inode_hash_lock;
> struct hlist_head inode_hashtable[EXFAT_HASH_SIZE];
> -#ifdef CONFIG_EXFAT_KERNEL_DEBUG
> +#ifdef CONFIG_STAGING_EXFAT_KERNEL_DEBUG
> long debug_flags;
> -#endif /* CONFIG_EXFAT_K
