[RFC PATCH] file: Wrap locking mechanism for f_pos_lock

[RFC PATCH] file: Wrap locking mechanism for f_pos_lock

[I Hsin Cheng]

As the implementation of "f->f_pos_lock" may change in the future,
wrapping the actual implementation of locking and unlocking of it can
provide better decoupling semantics.

"__f_unlock_pos()" already exist and does that, adding "__f_lock_pos()"
can provide full decoupling.

Signed-off-by: I Hsin Cheng <richard120310@gmail.com>
---
 fs/file.c            | 7 ++++++-
 include/linux/file.h | 1 +
 2 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/fs/file.c b/fs/file.c
index fb1011cf6b4a..b93ac67d276d 100644
--- a/fs/file.c
+++ b/fs/file.c
@@ -1181,6 +1181,11 @@ static inline bool file_needs_f_pos_lock(struct file *file)
 		(file_count(file) > 1 || file->f_op->iterate_shared);
 }
 
+void __f_lock_pos(struct file *f)
+{
+	mutex_lock(&f->f_pos_lock);
+}
+
 struct fd fdget_pos(unsigned int fd)
 {
 	struct fd f = fdget(fd);
@@ -1188,7 +1193,7 @@ struct fd fdget_pos(unsigned int fd)
 
 	if (file && file_needs_f_pos_lock(file)) {
 		f.word |= FDPUT_POS_UNLOCK;
-		mutex_lock(&file->f_pos_lock);
+		__f_lock_pos(file);
 	}
 	return f;
 }
diff --git a/include/linux/file.h b/include/linux/file.h
index 302f11355b10..16292bd95499 100644
--- a/include/linux/file.h
+++ b/include/linux/file.h
@@ -67,6 +67,7 @@ extern struct file *fget(unsigned int fd);
 extern struct file *fget_raw(unsigned int fd);
 extern struct file *fget_task(struct task_struct *task, unsigned int fd);
 extern struct file *fget_task_next(struct task_struct *task, unsigned int *fd);
+extern void __f_lock_pos(struct file *file);
 extern void __f_unlock_pos(struct file *);
 
 struct fd fdget(unsigned int fd);
-- 
2.43.0

Re: [RFC PATCH] file: Wrap locking mechanism for f_pos_lock

[Jan Kara]

On Wed 04-12-24 17:23:25, I Hsin Cheng wrote:
> As the implementation of "f->f_pos_lock" may change in the future,
> wrapping the actual implementation of locking and unlocking of it can
> provide better decoupling semantics.
> 
> "__f_unlock_pos()" already exist and does that, adding "__f_lock_pos()"
> can provide full decoupling.
> 
> Signed-off-by: I Hsin Cheng <richard120310@gmail.com>

I guess this would make sense for consistence. But Al, what was the
motivation of introducing __f_unlock_pos() in the first place? It has one
caller and was silently introduced in 63b6df14134d ("give
readdir(2)/getdents(2)/etc. uniform exclusion with lseek()") about 8 years
ago.

								Honza


> ---
>  fs/file.c            | 7 ++++++-
>  include/linux/file.h | 1 +
>  2 files changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/file.c b/fs/file.c
> index fb1011cf6b4a..b93ac67d276d 100644
> --- a/fs/file.c
> +++ b/fs/file.c
> @@ -1181,6 +1181,11 @@ static inline bool file_needs_f_pos_lock(struct file *file)
>  		(file_count(file) > 1 || file->f_op->iterate_shared);
>  }
>  
> +void __f_lock_pos(struct file *f)
> +{
> +	mutex_lock(&f->f_pos_lock);
> +}
> +
>  struct fd fdget_pos(unsigned int fd)
>  {
>  	struct fd f = fdget(fd);
> @@ -1188,7 +1193,7 @@ struct fd fdget_pos(unsigned int fd)
>  
>  	if (file && file_needs_f_pos_lock(file)) {
>  		f.word |= FDPUT_POS_UNLOCK;
> -		mutex_lock(&file->f_pos_lock);
> +		__f_lock_pos(file);
>  	}
>  	return f;
>  }
> diff --git a/include/linux/file.h b/include/linux/file.h
> index 302f11355b10..16292bd95499 100644
> --- a/include/linux/file.h
> +++ b/include/linux/file.h
> @@ -67,6 +67,7 @@ extern struct file *fget(unsigned int fd);
>  extern struct file *fget_raw(unsigned int fd);
>  extern struct file *fget_task(struct task_struct *task, unsigned int fd);
>  extern struct file *fget_task_next(struct task_struct *task, unsigned int *fd);
> +extern void __f_lock_pos(struct file *file);
>  extern void __f_unlock_pos(struct file *);
>  
>  struct fd fdget(unsigned int fd);
> -- 
> 2.43.0
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [RFC PATCH] file: Wrap locking mechanism for f_pos_lock

[Christian Brauner]

> motivation of introducing __f_unlock_pos() in the first place? It has one

May I venture a guess:

  CALL    ../scripts/checksyscalls.sh
  INSTALL libsubcmd_headers
  INSTALL libsubcmd_headers
  CC      fs/read_write.o
In file included from ../fs/read_write.c:12:
../include/linux/file.h:78:27: error: incomplete definition of type 'struct file'
   78 |                 mutex_unlock(&fd_file(f)->f_pos_lock);
      |                               ~~~~~~~~~~^

If you don't include linux/fs.h before linux/file.h you'd get compilation
errors and we don't want to include linux/fs.h in linux/file.h.

I wouldn't add another wrapper for lock though. Just put a comment on top of
__f_unlock_pos().       

Re: [RFC PATCH] file: Wrap locking mechanism for f_pos_lock

[Jan Kara]

On Wed 04-12-24 12:11:02, Christian Brauner wrote:
> > motivation of introducing __f_unlock_pos() in the first place? It has one
> 
> May I venture a guess:
> 
>   CALL    ../scripts/checksyscalls.sh
>   INSTALL libsubcmd_headers
>   INSTALL libsubcmd_headers
>   CC      fs/read_write.o
> In file included from ../fs/read_write.c:12:
> ../include/linux/file.h:78:27: error: incomplete definition of type 'struct file'
>    78 |                 mutex_unlock(&fd_file(f)->f_pos_lock);
>       |                               ~~~~~~~~~~^
> 
> If you don't include linux/fs.h before linux/file.h you'd get compilation
> errors and we don't want to include linux/fs.h in linux/file.h.

Ah, subtle ;)

> I wouldn't add another wrapper for lock though. Just put a comment on top of
> __f_unlock_pos().       

Yes, I guess comment is better in that case.

								Honza

-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [RFC PATCH] file: Wrap locking mechanism for f_pos_lock

[I Hsin Cheng]

On Wed, Dec 04, 2024 at 01:48:29PM +0100, Jan Kara wrote:
> On Wed 04-12-24 12:11:02, Christian Brauner wrote:
> > > motivation of introducing __f_unlock_pos() in the first place? It has one
> > 
> > May I venture a guess:
> > 
> >   CALL    ../scripts/checksyscalls.sh
> >   INSTALL libsubcmd_headers
> >   INSTALL libsubcmd_headers
> >   CC      fs/read_write.o
> > In file included from ../fs/read_write.c:12:
> > ../include/linux/file.h:78:27: error: incomplete definition of type 'struct file'
> >    78 |                 mutex_unlock(&fd_file(f)->f_pos_lock);
> >       |                               ~~~~~~~~~~^
> > 
> > If you don't include linux/fs.h before linux/file.h you'd get compilation
> > errors and we don't want to include linux/fs.h in linux/file.h.
> 
> Ah, subtle ;)
> 
> > I wouldn't add another wrapper for lock though. Just put a comment on top of
> > __f_unlock_pos().       
> 
> Yes, I guess comment is better in that case.
> 
> 								Honza
> 
> -- 
> Jan Kara <jack@suse.com>
> SUSE Labs, CR


No problem, I'll add comments on __f_unlock_pos() to explain for the
inconsistency then send a formal path.

But I want to ask what's the motivation of defining "fdput_pos()" as
static inline? If we make it "void fdput_pos()", we should be able to
write the implementation in file.c and thus can get rid of
"__f_unlock_pos()".

Is it just for the inline function speed up?

Best regards,
Richard Cheng.

Re: [RFC PATCH] file: Wrap locking mechanism for f_pos_lock

[Christian Brauner]

> Is it just for the inline function speed up?

Yes, very likely.

Re: [RFC PATCH] file: Wrap locking mechanism for f_pos_lock

[Al Viro]

On Wed, Dec 04, 2024 at 11:26:44AM +0100, Jan Kara wrote:
> On Wed 04-12-24 17:23:25, I Hsin Cheng wrote:
> > As the implementation of "f->f_pos_lock" may change in the future,
> > wrapping the actual implementation of locking and unlocking of it can
> > provide better decoupling semantics.
> > 
> > "__f_unlock_pos()" already exist and does that, adding "__f_lock_pos()"
> > can provide full decoupling.
> > 
> > Signed-off-by: I Hsin Cheng <richard120310@gmail.com>
> 
> I guess this would make sense for consistence. But Al, what was the
> motivation of introducing __f_unlock_pos() in the first place? It has one
> caller and was silently introduced in 63b6df14134d ("give
> readdir(2)/getdents(2)/etc. uniform exclusion with lseek()") about 8 years
> ago.

Encapsulation, actually.  Look:

* grabbing the lock without setting FDPUT_POS_UNLOCK should never happen;
fdget_pos() does handle that, no need for grabbing the lock as an operation
on existing struct fd instance

* dropping the lock is done in destructor; no need for separate "it may be
locked here" scope

* we want fdput_pos() to be inlined (and preferably eliminated in the case
of failed fdget_pos())

__f_lock_pos() would *break* encapsulation - any user of that thing would
have to deal with FDPUT_POS_UNLOCK bit and the rest of struct fd guts.