Re: [PATCH v3 2/4] ocfs2: sysfile interfaces for online file check

[Mark Fasheh]

On Fri, Dec 25, 2015 at 03:16:17PM +0800, Gang He wrote:
> Implement online file check sysfile interfaces, e.g.
> how to create the related sysfile according to device name,
> how to display/handle file check request from the sysfile.
> 
> Signed-off-by: Gang He <ghe@xxxxxxxx>

Most of this looks good, I have two comments below. Also thank you for
redoing the interface to be more sysfs friendly.


> diff --git a/fs/ocfs2/filecheck.c b/fs/ocfs2/filecheck.c
> new file mode 100644
> index 0000000..a83e4ba
> --- /dev/null
> +++ b/fs/ocfs2/filecheck.c
> @@ -0,0 +1,605 @@
> +static DEFINE_SPINLOCK(ocfs2_filecheck_sysfs_lock);
> +static LIST_HEAD(ocfs2_filecheck_sysfs_list);
> +
> +struct ocfs2_filecheck {
> +	struct list_head fc_head;	/* File check entry list head */
> +	spinlock_t fc_lock;
> +	unsigned int fc_max;	/* Maximum number of entry in list */

What is the point of fc_max? Only root can initiate file check so we need
not worry about a malicious user eating up our memory. That should let us
drop a bunch of the code below that is concerned with setting/reporting it.


> +	unsigned int fc_size;	/* Current entry count in list */
> +	unsigned int fc_done;	/* Finished entry count in list */
> +};
> +
> +struct ocfs2_filecheck_sysfs_entry {	/* sysfs entry per mounting */
> +	struct list_head fs_list;
> +	atomic_t fs_count;
> +	struct super_block *fs_sb;
> +	struct kset *fs_devicekset;
> +	struct kset *fs_fcheckkset;
> +	struct ocfs2_filecheck *fs_fcheck;
> +};
> +
> +#define OCFS2_FILECHECK_MAXSIZE		100
> +#define OCFS2_FILECHECK_MINSIZE		10
> +
> +/* File check operation type */
> +enum {
> +	OCFS2_FILECHECK_TYPE_CHK = 0,	/* Check a file(inode) */
> +	OCFS2_FILECHECK_TYPE_FIX,	/* Fix a file(inode) */
> +	OCFS2_FILECHECK_TYPE_SET = 100	/* Set entry list maximum size */
> +};
> +
> +struct ocfs2_filecheck_entry {
> +	struct list_head fe_list;
> +	unsigned long fe_ino;
> +	unsigned int fe_type;
> +	unsigned short fe_done:1;
> +	unsigned short fe_status:15;

I don't see the need to use a short here (or bitfield) for fc_status. IMHO
it is less error-prone if we just make it an int or unsigned int.


This is a bit off topic but I dream of the day when we can return errors
which userspace undestands but are outside the tiny range of 0-255  :)

Thanks,
	--Mark

--
Mark Fasheh