From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wm1-f66.google.com ([209.85.128.66]:54401 "EHLO mail-wm1-f66.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1727125AbeJYV3H (ORCPT ); Thu, 25 Oct 2018 17:29:07 -0400 Received: by mail-wm1-f66.google.com with SMTP id r63-v6so1448279wma.4 for ; Thu, 25 Oct 2018 05:56:26 -0700 (PDT) Date: Thu, 25 Oct 2018 13:56:23 +0100 From: Phillip Potter To: Jan Kara Cc: viro@zeniv.linux.org.uk, amir73il@gmail.com, linux-fsdevel@vger.kernel.org Subject: Re: [RFC][PATCH v3 01/10] fs: common implementation of file type Message-ID: <20181025125623.GA8951@pathfinder> References: <20181023201953.GA15687@pathfinder> <20181025112044.GA7711@quack2.suse.cz> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20181025112044.GA7711@quack2.suse.cz> Sender: linux-fsdevel-owner@vger.kernel.org List-ID: On Thu, Oct 25, 2018 at 01:20:44PM +0200, Jan Kara wrote: > On Tue 23-10-18 21:19:53, Phillip Potter wrote: > > Many file systems use a copy&paste implementation > > of dirent to on-disk file type conversions. > > > > Create a common implementation to be used by file systems > > with some useful conversion helpers to reduce open coded > > file type conversions in file system code. > > Thanks for the patch. I like the cleanup. Some comments inline... > > > diff --git a/include/linux/file_type.h b/include/linux/file_type.h > > new file mode 100644 > > index 000000000000..f015c41ca90c > > --- /dev/null > > +++ b/include/linux/file_type.h > > @@ -0,0 +1,108 @@ > > +/* SPDX-License-Identifier: GPL-2.0 */ > > +#ifndef _LINUX_FILE_TYPE_H > > +#define _LINUX_FILE_TYPE_H > > + > > +/* > > + * This is a common implementation of dirent to fs on-disk > > + * file type conversion. Although the fs on-disk bits are > > + * specific to every file system, in practice, many file systems > > + * use the exact same on-disk format to describe the lower 3 > > + * file type bits that represent the 7 POSIX file types. > > + * All those file systems can use this generic code for the > > + * conversions: > > + * i_mode -> fs on-disk file type (ftype) > > + * fs on-disk file type (ftype) -> dirent file type (dtype) > > + * i_mode -> dirent file type (dtype) > > I don't think these three lines above are really useful. If you want to > make it easier for fs developers, add Docbook coments at function > implementations. > > > + */ > > + > > +/* > > + * struct dirent file types > > + * exposed to user via getdents(2), readdir(3) > > + * > > + * These match bits 12..15 of stat.st_mode > > + * (ie "(i_mode >> 12) & 15"). > > + */ > > +#define S_DT_SHIFT 12 > > +#define S_DT(mode) (((mode) & S_IFMT) >> S_DT_SHIFT) > > +#define S_DT_MASK (S_IFMT >> S_DT_SHIFT) > > + > > +#define DT_UNKNOWN 0 > > +#define DT_FIFO S_DT(S_IFIFO) /* 1 */ > > +#define DT_CHR S_DT(S_IFCHR) /* 2 */ > > +#define DT_DIR S_DT(S_IFDIR) /* 4 */ > > +#define DT_BLK S_DT(S_IFBLK) /* 6 */ > > +#define DT_REG S_DT(S_IFREG) /* 8 */ > > +#define DT_LNK S_DT(S_IFLNK) /* 10 */ > > +#define DT_SOCK S_DT(S_IFSOCK) /* 12 */ > > Why not keep the original definition by absolute number. After all these > must match glibc... > > > +#define DT_WHT 14 > > + > > +#define DT_MAX (S_DT_MASK + 1) /* 16 */ > > + > > +/* > > + * fs on-disk file types. > > + * Only the low 3 bits are used for the POSIX file types. > > + * Other bits are reserved for fs private use. > > + * > > + * Note that no fs currently stores the whiteout type on-disk, > > + * so whiteout dirents are exposed to user as DT_CHR. > > + */ > > +#define FT_UNKNOWN 0 > > +#define FT_REG_FILE 1 > > +#define FT_DIR 2 > > +#define FT_CHRDEV 3 > > +#define FT_BLKDEV 4 > > +#define FT_FIFO 5 > > +#define FT_SOCK 6 > > +#define FT_SYMLINK 7 > > + > > +#define FT_MAX 8 > > + > > +/* > > + * fs on-disk file type to dirent file type conversion > > + */ > > +static unsigned char fs_dtype_by_ftype[FT_MAX] = { > > + [FT_UNKNOWN] = DT_UNKNOWN, > > + [FT_REG_FILE] = DT_REG, > > + [FT_DIR] = DT_DIR, > > + [FT_CHRDEV] = DT_CHR, > > + [FT_BLKDEV] = DT_BLK, > > + [FT_FIFO] = DT_FIFO, > > + [FT_SOCK] = DT_SOCK, > > + [FT_SYMLINK] = DT_LNK > > +}; > > So you define static variable in a header file. That will make it appear in > each and every object that includes this header (when not used it will get > optimized away but still...). IMO that is not good and I don't think > anything here is so performance super-crittical, that the cost of > additional function call would matter (correct me if I'm wrong here). So > I'd rather see these tables and associated functions in some C file. > > > +static inline unsigned char fs_dtype(int filetype) > > This function name is not very descriptive and consistent with the other > two. It should be like fs_ftype_to_dtype(), right? > > > +{ > > + if (filetype >= FT_MAX) > > + return DT_UNKNOWN; > > + > > + return fs_dtype_by_ftype[filetype]; > > +} > > Otherwise the patch looks good to me. > > Honza > -- > Jan Kara > SUSE Labs, CR Dear Jan, Thanks for your comments/feedback, really appreciate it. All good points and I will make sure I act on it all before republishing the series. I'd be interested to know your thoughts on Ted T'so's suggestion about moving the new header to the uapi area - yes or no in your opinion? Happy with either, but looking for the widest possible acceptance. Thanks. Regards, Phil