| Message ID | 20241204092325.170349-1-richard120310@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [RFC] file: Wrap locking mechanism for f_pos_lock | expand |
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 >
> 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().
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
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.
> Is it just for the inline function speed up?
Yes, very likely.
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.
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);
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(-)