[PATCH 1/9] fs: add fcntl() interface for setting/getting write life time hints

[Bart Van Assche]

On Mon, 2017-06-19 at 11:04 -0600, Jens Axboe wrote:
> +static long fcntl_rw_hint(struct file *file, unsigned int cmd,
> +			  u64 __user *ptr)
> +{
> +	struct inode *inode = file_inode(file);
> +	long ret = 0;
> +	u64 hint;
> +
> +	switch (cmd) {
> +	case F_GET_RW_HINT:
> +		hint = mask_to_write_hint(inode->i_flags, S_WRITE_LIFE_SHIFT);
> +		if (put_user(hint, ptr))
> +			ret = -EFAULT;
> +		break;
> +	case F_SET_RW_HINT:
> +		if (get_user(hint, ptr)) {
> +			ret = -EFAULT;
> +			break;
> +		}
> +		switch (hint) {
> +		case WRITE_LIFE_NONE:
> +		case WRITE_LIFE_SHORT:
> +		case WRITE_LIFE_MEDIUM:
> +		case WRITE_LIFE_LONG:
> +		case WRITE_LIFE_EXTREME:
> +			inode_set_write_hint(inode, hint);
> +			ret = 0;
> +			break;
> +		default:
> +			ret = -EINVAL;
> +		}
> +		break;
> +	default:
> +		ret = -EINVAL;
> +		break;
> +	}
> +
> +	return ret;
> +}

Hello Jens,

Do we need an (inline) helper function for checking the validity of a
numerical WRITE_LIFE value next to the definition of the WRITE_LIFE_*
constants, e.g. WRITE_LIFE_NONE <= hint && hint <= WRITE_LIFE_EXTREME?

> +/*
> + * Steal 3 bits for stream information, this allows 8 valid streams
> + */
> +#define IOCB_WRITE_LIFE_SHIFT	7
> +#define IOCB_WRITE_LIFE_MASK	(BIT(7) | BIT(8) | BIT(9))

A minor comment: how about making this easier to read by defining
IOCB_WRITE_LIFE_MASK as (7 << IOCB_WRITE_LIFE_SHIFT)?

>  /*
> + * Expected life time hint of a write for this inode. This uses the
> + * WRITE_LIFE_* encoding, we just need to define the shift. We need
> + * 3 bits for this. Next S_* value is 131072, bit 17.
> + */
> +#define S_WRITE_LIFE_MASK	0x1c000	/* bits 14..16 */
> +#define S_WRITE_LIFE_SHIFT	14	/* 16384, next bit */

Another minor comment: how about making this easier to read by defining
S_WRITE_LIFE_MASK as (7 << S_WRITE_LIFE_SHIFT)?

> /*
> + * Write life time hint values.
> + */
> +enum rw_hint {
> +	WRITE_LIFE_NONE = RWH_WRITE_LIFE_NONE,
> +	WRITE_LIFE_SHORT = RWH_WRITE_LIFE_SHORT,
> +	WRITE_LIFE_MEDIUM = RWH_WRITE_LIFE_MEDIUM,
> +	WRITE_LIFE_LONG = RWH_WRITE_LIFE_LONG,
> +	WRITE_LIFE_EXTREME = RWH_WRITE_LIFE_EXTREME
> +};
> [ ... ]
> +/*
> + * Valid hint values for F_{GET,SET}_RW_HINT
> + */
> +#define RWH_WRITE_LIFE_NONE	0
> +#define RWH_WRITE_LIFE_SHORT	1
> +#define RWH_WRITE_LIFE_MEDIUM	2
> +#define RWH_WRITE_LIFE_LONG	3
> +#define RWH_WRITE_LIFE_EXTREME	4

Maybe I missed something, but it's not clear to me why we have both an enum and
defines with the same numerical values? BTW, I prefer an enum above #defines.

Thanks,

Bart.