remove write hint leftovers v2

remove write hint leftovers v2

[Christoph Hellwig]

Hi Jens,

this removes the other two fields and the two now unused fcntls
as pointed out by Dave.

Changes since v1:
 - better commit log

[PATCH 1/2] fs: remove kiocb.ki_hint

[Christoph Hellwig]

This field is entirely unused now except for a tracepoint in f2fs, so
remove it.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/aio.c                    |  1 -
 fs/cachefiles/io.c          |  2 --
 fs/f2fs/file.c              |  6 ------
 fs/io_uring.c               |  1 -
 include/linux/fs.h          | 12 ------------
 include/trace/events/f2fs.h |  3 +--
 6 files changed, 1 insertion(+), 24 deletions(-)

diff --git a/fs/aio.c b/fs/aio.c
index 4ceba13a7db0f..eb0948bb74f18 100644
--- a/fs/aio.c
+++ b/fs/aio.c
@@ -1478,7 +1478,6 @@ static int aio_prep_rw(struct kiocb *req, const struct iocb *iocb)
 	req->ki_flags = iocb_flags(req->ki_filp);
 	if (iocb->aio_flags & IOCB_FLAG_RESFD)
 		req->ki_flags |= IOCB_EVENTFD;
-	req->ki_hint = ki_hint_validate(file_write_hint(req->ki_filp));
 	if (iocb->aio_flags & IOCB_FLAG_IOPRIO) {
 		/*
 		 * If the IOCB_FLAG_IOPRIO flag of aio_flags is set, then
diff --git a/fs/cachefiles/io.c b/fs/cachefiles/io.c
index 753986ea1583b..bc7c7a7d92600 100644
--- a/fs/cachefiles/io.c
+++ b/fs/cachefiles/io.c
@@ -138,7 +138,6 @@ static int cachefiles_read(struct netfs_cache_resources *cres,
 	ki->iocb.ki_filp	= file;
 	ki->iocb.ki_pos		= start_pos + skipped;
 	ki->iocb.ki_flags	= IOCB_DIRECT;
-	ki->iocb.ki_hint	= ki_hint_validate(file_write_hint(file));
 	ki->iocb.ki_ioprio	= get_current_ioprio();
 	ki->skipped		= skipped;
 	ki->object		= object;
@@ -313,7 +312,6 @@ static int cachefiles_write(struct netfs_cache_resources *cres,
 	ki->iocb.ki_filp	= file;
 	ki->iocb.ki_pos		= start_pos;
 	ki->iocb.ki_flags	= IOCB_DIRECT | IOCB_WRITE;
-	ki->iocb.ki_hint	= ki_hint_validate(file_write_hint(file));
 	ki->iocb.ki_ioprio	= get_current_ioprio();
 	ki->object		= object;
 	ki->inval_counter	= cres->inval_counter;
diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
index 3c98ef6af97d1..45076c01a2bab 100644
--- a/fs/f2fs/file.c
+++ b/fs/f2fs/file.c
@@ -4479,10 +4479,8 @@ static ssize_t f2fs_dio_write_iter(struct kiocb *iocb, struct iov_iter *from,
 	struct f2fs_inode_info *fi = F2FS_I(inode);
 	struct f2fs_sb_info *sbi = F2FS_I_SB(inode);
 	const bool do_opu = f2fs_lfs_mode(sbi);
-	const int whint_mode = F2FS_OPTION(sbi).whint_mode;
 	const loff_t pos = iocb->ki_pos;
 	const ssize_t count = iov_iter_count(from);
-	const enum rw_hint hint = iocb->ki_hint;
 	unsigned int dio_flags;
 	struct iomap_dio *dio;
 	ssize_t ret;
@@ -4515,8 +4513,6 @@ static ssize_t f2fs_dio_write_iter(struct kiocb *iocb, struct iov_iter *from,
 		if (do_opu)
 			down_read(&fi->i_gc_rwsem[READ]);
 	}
-	if (whint_mode == WHINT_MODE_OFF)
-		iocb->ki_hint = WRITE_LIFE_NOT_SET;
 
 	/*
 	 * We have to use __iomap_dio_rw() and iomap_dio_complete() instead of
@@ -4539,8 +4535,6 @@ static ssize_t f2fs_dio_write_iter(struct kiocb *iocb, struct iov_iter *from,
 		ret = iomap_dio_complete(dio);
 	}
 
-	if (whint_mode == WHINT_MODE_OFF)
-		iocb->ki_hint = hint;
 	if (do_opu)
 		up_read(&fi->i_gc_rwsem[READ]);
 	up_read(&fi->i_gc_rwsem[WRITE]);
diff --git a/fs/io_uring.c b/fs/io_uring.c
index 23e7f93d39563..02400fd00501e 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -3790,7 +3790,6 @@ static int io_write_prep(struct io_kiocb *req, const struct io_uring_sqe *sqe)
 {
 	if (unlikely(!(req->file->f_mode & FMODE_WRITE)))
 		return -EBADF;
-	req->rw.kiocb.ki_hint = ki_hint_validate(file_write_hint(req->file));
 	return io_prep_rw(req, sqe);
 }
 
diff --git a/include/linux/fs.h b/include/linux/fs.h
index e2d892b201b07..d5658ac5d8c65 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -327,7 +327,6 @@ struct kiocb {
 	void (*ki_complete)(struct kiocb *iocb, long ret);
 	void			*private;
 	int			ki_flags;
-	u16			ki_hint;
 	u16			ki_ioprio; /* See linux/ioprio.h */
 	struct wait_page_queue	*ki_waitq; /* for async buffered IO */
 	randomized_struct_fields_end
@@ -2225,21 +2224,11 @@ static inline enum rw_hint file_write_hint(struct file *file)
 
 static inline int iocb_flags(struct file *file);
 
-static inline u16 ki_hint_validate(enum rw_hint hint)
-{
-	typeof(((struct kiocb *)0)->ki_hint) max_hint = -1;
-
-	if (hint <= max_hint)
-		return hint;
-	return 0;
-}
-
 static inline void init_sync_kiocb(struct kiocb *kiocb, struct file *filp)
 {
 	*kiocb = (struct kiocb) {
 		.ki_filp = filp,
 		.ki_flags = iocb_flags(filp),
-		.ki_hint = ki_hint_validate(file_write_hint(filp)),
 		.ki_ioprio = get_current_ioprio(),
 	};
 }
@@ -2250,7 +2239,6 @@ static inline void kiocb_clone(struct kiocb *kiocb, struct kiocb *kiocb_src,
 	*kiocb = (struct kiocb) {
 		.ki_filp = filp,
 		.ki_flags = kiocb_src->ki_flags,
-		.ki_hint = kiocb_src->ki_hint,
 		.ki_ioprio = kiocb_src->ki_ioprio,
 		.ki_pos = kiocb_src->ki_pos,
 	};
diff --git a/include/trace/events/f2fs.h b/include/trace/events/f2fs.h
index f701bb23f83c4..1779e133cea0c 100644
--- a/include/trace/events/f2fs.h
+++ b/include/trace/events/f2fs.h
@@ -956,12 +956,11 @@ TRACE_EVENT(f2fs_direct_IO_enter,
 		__entry->rw	= rw;
 	),
 
-	TP_printk("dev = (%d,%d), ino = %lu pos = %lld len = %lu ki_flags = %x ki_hint = %x ki_ioprio = %x rw = %d",
+	TP_printk("dev = (%d,%d), ino = %lu pos = %lld len = %lu ki_flags = %x ki_ioprio = %x rw = %d",
 		show_dev_ino(__entry),
 		__entry->iocb->ki_pos,
 		__entry->len,
 		__entry->iocb->ki_flags,
-		__entry->iocb->ki_hint,
 		__entry->iocb->ki_ioprio,
 		__entry->rw)
 );
-- 
2.30.2

[PATCH 2/2] fs: remove fs.f_write_hint

[Christoph Hellwig]

The value is now completely unused except for reporting it back through
the F_GET_FILE_RW_HINT ioctl, so remove the value and the two ioctls
for it.

Trying to use the F_SET_FILE_RW_HINT and F_GET_FILE_RW_HINT fcntls will
now return EINVAL, just like it would on a kernel that never supported
this functionality in the first place.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/fcntl.c         | 18 ------------------
 fs/open.c          |  1 -
 include/linux/fs.h |  9 ---------
 3 files changed, 28 deletions(-)

diff --git a/fs/fcntl.c b/fs/fcntl.c
index 9c6c6a3e2de51..f15d885b97961 100644
--- a/fs/fcntl.c
+++ b/fs/fcntl.c
@@ -291,22 +291,6 @@ static long fcntl_rw_hint(struct file *file, unsigned int cmd,
 	u64 h;
 
 	switch (cmd) {
-	case F_GET_FILE_RW_HINT:
-		h = file_write_hint(file);
-		if (copy_to_user(argp, &h, sizeof(*argp)))
-			return -EFAULT;
-		return 0;
-	case F_SET_FILE_RW_HINT:
-		if (copy_from_user(&h, argp, sizeof(h)))
-			return -EFAULT;
-		hint = (enum rw_hint) h;
-		if (!rw_hint_valid(hint))
-			return -EINVAL;
-
-		spin_lock(&file->f_lock);
-		file->f_write_hint = hint;
-		spin_unlock(&file->f_lock);
-		return 0;
 	case F_GET_RW_HINT:
 		h = inode->i_write_hint;
 		if (copy_to_user(argp, &h, sizeof(*argp)))
@@ -431,8 +415,6 @@ static long do_fcntl(int fd, unsigned int cmd, unsigned long arg,
 		break;
 	case F_GET_RW_HINT:
 	case F_SET_RW_HINT:
-	case F_GET_FILE_RW_HINT:
-	case F_SET_FILE_RW_HINT:
 		err = fcntl_rw_hint(filp, cmd, arg);
 		break;
 	default:
diff --git a/fs/open.c b/fs/open.c
index 9ff2f621b760b..1315253e02473 100644
--- a/fs/open.c
+++ b/fs/open.c
@@ -835,7 +835,6 @@ static int do_dentry_open(struct file *f,
 	     likely(f->f_op->write || f->f_op->write_iter))
 		f->f_mode |= FMODE_CAN_WRITE;
 
-	f->f_write_hint = WRITE_LIFE_NOT_SET;
 	f->f_flags &= ~(O_CREAT | O_EXCL | O_NOCTTY | O_TRUNC);
 
 	file_ra_state_init(&f->f_ra, f->f_mapping->host->i_mapping);
diff --git a/include/linux/fs.h b/include/linux/fs.h
index d5658ac5d8c65..a1fc3b41cd82f 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -966,7 +966,6 @@ struct file {
 	 * Must not be taken from IRQ context.
 	 */
 	spinlock_t		f_lock;
-	enum rw_hint		f_write_hint;
 	atomic_long_t		f_count;
 	unsigned int 		f_flags;
 	fmode_t			f_mode;
@@ -2214,14 +2213,6 @@ static inline bool HAS_UNMAPPED_ID(struct user_namespace *mnt_userns,
 	       !gid_valid(i_gid_into_mnt(mnt_userns, inode));
 }
 
-static inline enum rw_hint file_write_hint(struct file *file)
-{
-	if (file->f_write_hint != WRITE_LIFE_NOT_SET)
-		return file->f_write_hint;
-
-	return file_inode(file)->i_write_hint;
-}
-
 static inline int iocb_flags(struct file *file);
 
 static inline void init_sync_kiocb(struct kiocb *kiocb, struct file *filp)
-- 
2.30.2

Re: [PATCH 1/2] fs: remove kiocb.ki_hint

[Chaitanya Kulkarni]

On 3/7/22 22:05, Christoph Hellwig wrote:
> This field is entirely unused now except for a tracepoint in f2fs, so
> remove it.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---

Looks good.

Reviewed-by: Chaitanya Kulkarni <kch@nvidia.com>

Re: [PATCH 2/2] fs: remove fs.f_write_hint

[Chaitanya Kulkarni]

On 3/7/22 22:05, Christoph Hellwig wrote:
> The value is now completely unused except for reporting it back through
> the F_GET_FILE_RW_HINT ioctl, so remove the value and the two ioctls
> for it.
> 
> Trying to use the F_SET_FILE_RW_HINT and F_GET_FILE_RW_HINT fcntls will
> now return EINVAL, just like it would on a kernel that never supported
> this functionality in the first place.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>   fs/fcntl.c         | 18 ------------------

this follows the discussion on the other tread of returning EINVAL.

Looks good.

Reviewed-by: Chaitanya Kulkarni <kch@nvidia.com>

Re: [PATCH 1/2] fs: remove kiocb.ki_hint

[Dave Chinner]

On Tue, Mar 08, 2022 at 07:05:28AM +0100, Christoph Hellwig wrote:
> This field is entirely unused now except for a tracepoint in f2fs, so
> remove it.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Looks good.

Reviewed-by: Dave Chinner <dchinner@redhat.com>
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH 2/2] fs: remove fs.f_write_hint

[Dave Chinner]

On Tue, Mar 08, 2022 at 07:05:29AM +0100, Christoph Hellwig wrote:
> The value is now completely unused except for reporting it back through
> the F_GET_FILE_RW_HINT ioctl, so remove the value and the two ioctls
> for it.
> 
> Trying to use the F_SET_FILE_RW_HINT and F_GET_FILE_RW_HINT fcntls will
> now return EINVAL, just like it would on a kernel that never supported
> this functionality in the first place.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

LGTM.

Reviewed-by: Dave Chinner <dchinner@redhat.com>
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH 1/2] fs: remove kiocb.ki_hint

[Jens Axboe]

On Tue, 8 Mar 2022 07:05:28 +0100, Christoph Hellwig wrote:
> This field is entirely unused now except for a tracepoint in f2fs, so
> remove it.
> 
> 

Applied, thanks!

[1/2] fs: remove kiocb.ki_hint
      commit: 41d36a9f3e5336f5b48c3adba0777b8e217020d7
[2/2] fs: remove fs.f_write_hint
      commit: 7b12e49669c99f63bc12351c57e581f1f14d4adf

Best regards,
-- 
Jens Axboe

Re: [PATCH 1/2] fs: remove kiocb.ki_hint

[Jens Axboe]

On 3/8/22 5:55 PM, Jens Axboe wrote:
> On Tue, 8 Mar 2022 07:05:28 +0100, Christoph Hellwig wrote:
>> This field is entirely unused now except for a tracepoint in f2fs, so
>> remove it.
>>
>>
> 
> Applied, thanks!
> 
> [1/2] fs: remove kiocb.ki_hint
>       commit: 41d36a9f3e5336f5b48c3adba0777b8e217020d7
> [2/2] fs: remove fs.f_write_hint
>       commit: 7b12e49669c99f63bc12351c57e581f1f14d4adf

Upon thinking about the EINVAL solution a bit more, I do have a one
worry - if you're currently using write_hints in your application,
nobody should expect upgrading the kernel to break it. It's a fine
solution for anything else, but that particular point does annoy me.

So perhaps it is better after all to simply pretend we set the
hint just fine? That should always be safe.

What do you think?

-- 
Jens Axboe

Re: [PATCH 1/2] fs: remove kiocb.ki_hint

[Matthew Wilcox]

On Mon, Mar 21, 2022 at 08:13:10PM -0600, Jens Axboe wrote:
> On 3/8/22 5:55 PM, Jens Axboe wrote:
> > On Tue, 8 Mar 2022 07:05:28 +0100, Christoph Hellwig wrote:
> >> This field is entirely unused now except for a tracepoint in f2fs, so
> >> remove it.
> >>
> >>
> > 
> > Applied, thanks!
> > 
> > [1/2] fs: remove kiocb.ki_hint
> >       commit: 41d36a9f3e5336f5b48c3adba0777b8e217020d7
> > [2/2] fs: remove fs.f_write_hint
> >       commit: 7b12e49669c99f63bc12351c57e581f1f14d4adf
> 
> Upon thinking about the EINVAL solution a bit more, I do have a one
> worry - if you're currently using write_hints in your application,
> nobody should expect upgrading the kernel to break it. It's a fine
> solution for anything else, but that particular point does annoy me.

But if your application is run on an earlier kernel, it'll get -EINVAL.
So it must already be prepared to deal with that?

Re: [PATCH 1/2] fs: remove kiocb.ki_hint

[Jens Axboe]

On 3/21/22 8:45 PM, Matthew Wilcox wrote:
> On Mon, Mar 21, 2022 at 08:13:10PM -0600, Jens Axboe wrote:
>> On 3/8/22 5:55 PM, Jens Axboe wrote:
>>> On Tue, 8 Mar 2022 07:05:28 +0100, Christoph Hellwig wrote:
>>>> This field is entirely unused now except for a tracepoint in f2fs, so
>>>> remove it.
>>>>
>>>>
>>>
>>> Applied, thanks!
>>>
>>> [1/2] fs: remove kiocb.ki_hint
>>>       commit: 41d36a9f3e5336f5b48c3adba0777b8e217020d7
>>> [2/2] fs: remove fs.f_write_hint
>>>       commit: 7b12e49669c99f63bc12351c57e581f1f14d4adf
>>
>> Upon thinking about the EINVAL solution a bit more, I do have a one
>> worry - if you're currently using write_hints in your application,
>> nobody should expect upgrading the kernel to break it. It's a fine
>> solution for anything else, but that particular point does annoy me.
> 
> But if your application is run on an earlier kernel, it'll get
> -EINVAL. So it must already be prepared to deal with that?

Since support wasn't there, it's not unreasonable to expect that an
application was written on a newer kernel. Might just be an in-house or
application thing, who knows? But the point is that upgrading from 5.x
to 5.x+1, nobody should expect their application to break. And it will
with this change. If you downgrade a kernel and a feature didn't exist
back then, it's reasonable to expect that things may break.

Maybe this is being overly cautious, but... As a matter of principle,
it's unquestionably wrong.

We can just let Linus make the decision here, arming him with the info
he needs to make it in terms of hardware support. I'll write that up in
the pull request.

-- 
Jens Axboe

Re: [PATCH 1/2] fs: remove kiocb.ki_hint

[Keith Busch]

On Mon, Mar 21, 2022 at 08:50:10PM -0600, Jens Axboe wrote:
> On 3/21/22 8:45 PM, Matthew Wilcox wrote:
> > On Mon, Mar 21, 2022 at 08:13:10PM -0600, Jens Axboe wrote:
> >> On 3/8/22 5:55 PM, Jens Axboe wrote:
> >>> On Tue, 8 Mar 2022 07:05:28 +0100, Christoph Hellwig wrote:
> >>>> This field is entirely unused now except for a tracepoint in f2fs, so
> >>>> remove it.
> >>>>
> >>>>
> >>>
> >>> Applied, thanks!
> >>>
> >>> [1/2] fs: remove kiocb.ki_hint
> >>>       commit: 41d36a9f3e5336f5b48c3adba0777b8e217020d7
> >>> [2/2] fs: remove fs.f_write_hint
> >>>       commit: 7b12e49669c99f63bc12351c57e581f1f14d4adf
> >>
> >> Upon thinking about the EINVAL solution a bit more, I do have a one
> >> worry - if you're currently using write_hints in your application,
> >> nobody should expect upgrading the kernel to break it. It's a fine
> >> solution for anything else, but that particular point does annoy me.
> > 
> > But if your application is run on an earlier kernel, it'll get
> > -EINVAL. So it must already be prepared to deal with that?
> 
> Since support wasn't there, it's not unreasonable to expect that an
> application was written on a newer kernel. Might just be an in-house or
> application thing, who knows? But the point is that upgrading from 5.x
> to 5.x+1, nobody should expect their application to break. And it will
> with this change. If you downgrade a kernel and a feature didn't exist
> back then, it's reasonable to expect that things may break.
> 
> Maybe this is being overly cautious, but... As a matter of principle,
> it's unquestionably wrong.
> 
> We can just let Linus make the decision here, arming him with the info
> he needs to make it in terms of hardware support. I'll write that up in
> the pull request.

It's really just an advisory hint at the hardware level anyway, so
returning success without actually doing anything with it technically
satisfies the requirement.

Re: [PATCH 1/2] fs: remove kiocb.ki_hint

[Jens Axboe]

On 3/21/22 8:57 PM, Keith Busch wrote:
> On Mon, Mar 21, 2022 at 08:50:10PM -0600, Jens Axboe wrote:
>> On 3/21/22 8:45 PM, Matthew Wilcox wrote:
>>> On Mon, Mar 21, 2022 at 08:13:10PM -0600, Jens Axboe wrote:
>>>> On 3/8/22 5:55 PM, Jens Axboe wrote:
>>>>> On Tue, 8 Mar 2022 07:05:28 +0100, Christoph Hellwig wrote:
>>>>>> This field is entirely unused now except for a tracepoint in f2fs, so
>>>>>> remove it.
>>>>>>
>>>>>>
>>>>>
>>>>> Applied, thanks!
>>>>>
>>>>> [1/2] fs: remove kiocb.ki_hint
>>>>>       commit: 41d36a9f3e5336f5b48c3adba0777b8e217020d7
>>>>> [2/2] fs: remove fs.f_write_hint
>>>>>       commit: 7b12e49669c99f63bc12351c57e581f1f14d4adf
>>>>
>>>> Upon thinking about the EINVAL solution a bit more, I do have a one
>>>> worry - if you're currently using write_hints in your application,
>>>> nobody should expect upgrading the kernel to break it. It's a fine
>>>> solution for anything else, but that particular point does annoy me.
>>>
>>> But if your application is run on an earlier kernel, it'll get
>>> -EINVAL. So it must already be prepared to deal with that?
>>
>> Since support wasn't there, it's not unreasonable to expect that an
>> application was written on a newer kernel. Might just be an in-house or
>> application thing, who knows? But the point is that upgrading from 5.x
>> to 5.x+1, nobody should expect their application to break. And it will
>> with this change. If you downgrade a kernel and a feature didn't exist
>> back then, it's reasonable to expect that things may break.
>>
>> Maybe this is being overly cautious, but... As a matter of principle,
>> it's unquestionably wrong.
>>
>> We can just let Linus make the decision here, arming him with the info
>> he needs to make it in terms of hardware support. I'll write that up in
>> the pull request.
> 
> It's really just an advisory hint at the hardware level anyway, so
> returning success without actually doing anything with it technically
> satisfies the requirement.

Yes that's my point, nobody will know that the hints do nothing, all I
care about here is that the user ABI doesn't change. I know I originally
advocated for -EINVAL and I do think it's correct for _most_ cases, it's
just that it does have the potential to mess up a legit kernel upgrade
scenario which is exactly the kind of thing we don't want to happen.

-- 
Jens Axboe