2023-06-20 10:24:42

by stsp

[permalink] [raw]
Subject: [PATCH 1/3] fs/locks: F_UNLCK extension for F_OFD_GETLK

Currently F_UNLCK with F_OFD_GETLK returns -EINVAL.
The proposed extension allows to use it for getting the lock
information from the particular fd.

Signed-off-by: Stas Sergeev <[email protected]>

CC: Jeff Layton <[email protected]>
CC: Chuck Lever <[email protected]>
CC: Alexander Viro <[email protected]>
CC: Christian Brauner <[email protected]>
CC: [email protected]
CC: [email protected]

---
fs/locks.c | 23 ++++++++++++++++++++---
1 file changed, 20 insertions(+), 3 deletions(-)

diff --git a/fs/locks.c b/fs/locks.c
index df8b26a42524..210766007e63 100644
--- a/fs/locks.c
+++ b/fs/locks.c
@@ -868,6 +868,21 @@ static bool posix_locks_conflict(struct file_lock *caller_fl,
return locks_conflict(caller_fl, sys_fl);
}

+/* Determine if lock sys_fl blocks lock caller_fl. Used on xx_GETLK
+ * path so checks for additional GETLK-specific things like F_UNLCK.
+ */
+static bool posix_test_locks_conflict(struct file_lock *caller_fl,
+ struct file_lock *sys_fl)
+{
+ /* F_UNLCK checks any locks on the same fd. */
+ if (caller_fl->fl_type == F_UNLCK) {
+ if (!posix_same_owner(caller_fl, sys_fl))
+ return false;
+ return locks_overlap(caller_fl, sys_fl);
+ }
+ return posix_locks_conflict(caller_fl, sys_fl);
+}
+
/* Determine if lock sys_fl blocks lock caller_fl. FLOCK specific
* checking before calling the locks_conflict().
*/
@@ -901,7 +916,7 @@ posix_test_lock(struct file *filp, struct file_lock *fl)
retry:
spin_lock(&ctx->flc_lock);
list_for_each_entry(cfl, &ctx->flc_posix, fl_list) {
- if (!posix_locks_conflict(fl, cfl))
+ if (!posix_test_locks_conflict(fl, cfl))
continue;
if (cfl->fl_lmops && cfl->fl_lmops->lm_lock_expirable
&& (*cfl->fl_lmops->lm_lock_expirable)(cfl)) {
@@ -2207,7 +2222,8 @@ int fcntl_getlk(struct file *filp, unsigned int cmd, struct flock *flock)
if (fl == NULL)
return -ENOMEM;
error = -EINVAL;
- if (flock->l_type != F_RDLCK && flock->l_type != F_WRLCK)
+ if (cmd != F_OFD_GETLK && flock->l_type != F_RDLCK
+ && flock->l_type != F_WRLCK)
goto out;

error = flock_to_posix_lock(filp, fl, flock);
@@ -2414,7 +2430,8 @@ int fcntl_getlk64(struct file *filp, unsigned int cmd, struct flock64 *flock)
return -ENOMEM;

error = -EINVAL;
- if (flock->l_type != F_RDLCK && flock->l_type != F_WRLCK)
+ if (cmd != F_OFD_GETLK && flock->l_type != F_RDLCK
+ && flock->l_type != F_WRLCK)
goto out;

error = flock64_to_posix_lock(filp, fl, flock);
--
2.39.2



2023-06-20 11:14:34

by Jeffrey Layton

[permalink] [raw]
Subject: Re: [PATCH 1/3] fs/locks: F_UNLCK extension for F_OFD_GETLK

On Tue, 2023-06-20 at 14:55 +0500, Stas Sergeev wrote:
> Currently F_UNLCK with F_OFD_GETLK returns -EINVAL.
> The proposed extension allows to use it for getting the lock
> information from the particular fd.
>
> Signed-off-by: Stas Sergeev <[email protected]>
>
> CC: Jeff Layton <[email protected]>
> CC: Chuck Lever <[email protected]>
> CC: Alexander Viro <[email protected]>
> CC: Christian Brauner <[email protected]>
> CC: [email protected]
> CC: [email protected]
>
> ---
> fs/locks.c | 23 ++++++++++++++++++++---
> 1 file changed, 20 insertions(+), 3 deletions(-)
>
> diff --git a/fs/locks.c b/fs/locks.c
> index df8b26a42524..210766007e63 100644
> --- a/fs/locks.c
> +++ b/fs/locks.c
> @@ -868,6 +868,21 @@ static bool posix_locks_conflict(struct file_lock *caller_fl,
> return locks_conflict(caller_fl, sys_fl);
> }
>
> +/* Determine if lock sys_fl blocks lock caller_fl. Used on xx_GETLK
> + * path so checks for additional GETLK-specific things like F_UNLCK.
> + */
> +static bool posix_test_locks_conflict(struct file_lock *caller_fl,
> + struct file_lock *sys_fl)
> +{
> + /* F_UNLCK checks any locks on the same fd. */
> + if (caller_fl->fl_type == F_UNLCK) {
> + if (!posix_same_owner(caller_fl, sys_fl))
> + return false;
> + return locks_overlap(caller_fl, sys_fl);
> + }
> + return posix_locks_conflict(caller_fl, sys_fl);
> +}
> +
> /* Determine if lock sys_fl blocks lock caller_fl. FLOCK specific
> * checking before calling the locks_conflict().
> */
> @@ -901,7 +916,7 @@ posix_test_lock(struct file *filp, struct file_lock *fl)
> retry:
> spin_lock(&ctx->flc_lock);
> list_for_each_entry(cfl, &ctx->flc_posix, fl_list) {
> - if (!posix_locks_conflict(fl, cfl))
> + if (!posix_test_locks_conflict(fl, cfl))
> continue;
> if (cfl->fl_lmops && cfl->fl_lmops->lm_lock_expirable
> && (*cfl->fl_lmops->lm_lock_expirable)(cfl)) {
> @@ -2207,7 +2222,8 @@ int fcntl_getlk(struct file *filp, unsigned int cmd, struct flock *flock)
> if (fl == NULL)
> return -ENOMEM;
> error = -EINVAL;
> - if (flock->l_type != F_RDLCK && flock->l_type != F_WRLCK)
> + if (cmd != F_OFD_GETLK && flock->l_type != F_RDLCK
> + && flock->l_type != F_WRLCK)
> goto out;
>
> error = flock_to_posix_lock(filp, fl, flock);
> @@ -2414,7 +2430,8 @@ int fcntl_getlk64(struct file *filp, unsigned int cmd, struct flock64 *flock)
> return -ENOMEM;
>
> error = -EINVAL;
> - if (flock->l_type != F_RDLCK && flock->l_type != F_WRLCK)
> + if (cmd != F_OFD_GETLK && flock->l_type != F_RDLCK
> + && flock->l_type != F_WRLCK)
> goto out;
>
> error = flock64_to_posix_lock(filp, fl, flock);

This seems like a reasonable sort of interface to add, particularly for
the CRIU case. Using F_UNLCK for this is a bit kludgey, but adding a new
constant is probably worse.

I'm willing to take this in with an eye toward v6.6. Are you also
willing to draft up some manpage patches that detail this new interface?

--
Jeff Layton <[email protected]>

2023-06-20 11:20:30

by stsp

[permalink] [raw]
Subject: Re: [PATCH 1/3] fs/locks: F_UNLCK extension for F_OFD_GETLK

Hello,

20.06.2023 15:46, Jeff Layton пишет:
> On Tue, 2023-06-20 at 14:55 +0500, Stas Sergeev wrote:
>> Currently F_UNLCK with F_OFD_GETLK returns -EINVAL.
>> The proposed extension allows to use it for getting the lock
>> information from the particular fd.
>>
>> Signed-off-by: Stas Sergeev <[email protected]>
>>
>> CC: Jeff Layton <[email protected]>
>> CC: Chuck Lever <[email protected]>
>> CC: Alexander Viro <[email protected]>
>> CC: Christian Brauner <[email protected]>
>> CC: [email protected]
>> CC: [email protected]
>>
>> ---
>> fs/locks.c | 23 ++++++++++++++++++++---
>> 1 file changed, 20 insertions(+), 3 deletions(-)
>>
>> diff --git a/fs/locks.c b/fs/locks.c
>> index df8b26a42524..210766007e63 100644
>> --- a/fs/locks.c
>> +++ b/fs/locks.c
>> @@ -868,6 +868,21 @@ static bool posix_locks_conflict(struct file_lock *caller_fl,
>> return locks_conflict(caller_fl, sys_fl);
>> }
>>
>> +/* Determine if lock sys_fl blocks lock caller_fl. Used on xx_GETLK
>> + * path so checks for additional GETLK-specific things like F_UNLCK.
>> + */
>> +static bool posix_test_locks_conflict(struct file_lock *caller_fl,
>> + struct file_lock *sys_fl)
>> +{
>> + /* F_UNLCK checks any locks on the same fd. */
>> + if (caller_fl->fl_type == F_UNLCK) {
>> + if (!posix_same_owner(caller_fl, sys_fl))
>> + return false;
>> + return locks_overlap(caller_fl, sys_fl);
>> + }
>> + return posix_locks_conflict(caller_fl, sys_fl);
>> +}
>> +
>> /* Determine if lock sys_fl blocks lock caller_fl. FLOCK specific
>> * checking before calling the locks_conflict().
>> */
>> @@ -901,7 +916,7 @@ posix_test_lock(struct file *filp, struct file_lock *fl)
>> retry:
>> spin_lock(&ctx->flc_lock);
>> list_for_each_entry(cfl, &ctx->flc_posix, fl_list) {
>> - if (!posix_locks_conflict(fl, cfl))
>> + if (!posix_test_locks_conflict(fl, cfl))
>> continue;
>> if (cfl->fl_lmops && cfl->fl_lmops->lm_lock_expirable
>> && (*cfl->fl_lmops->lm_lock_expirable)(cfl)) {
>> @@ -2207,7 +2222,8 @@ int fcntl_getlk(struct file *filp, unsigned int cmd, struct flock *flock)
>> if (fl == NULL)
>> return -ENOMEM;
>> error = -EINVAL;
>> - if (flock->l_type != F_RDLCK && flock->l_type != F_WRLCK)
>> + if (cmd != F_OFD_GETLK && flock->l_type != F_RDLCK
>> + && flock->l_type != F_WRLCK)
>> goto out;
>>
>> error = flock_to_posix_lock(filp, fl, flock);
>> @@ -2414,7 +2430,8 @@ int fcntl_getlk64(struct file *filp, unsigned int cmd, struct flock64 *flock)
>> return -ENOMEM;
>>
>> error = -EINVAL;
>> - if (flock->l_type != F_RDLCK && flock->l_type != F_WRLCK)
>> + if (cmd != F_OFD_GETLK && flock->l_type != F_RDLCK
>> + && flock->l_type != F_WRLCK)
>> goto out;
>>
>> error = flock64_to_posix_lock(filp, fl, flock);
> This seems like a reasonable sort of interface to add, particularly for
> the CRIU case.

Just for the record: my own cases are
the remaining 2. CRIU case is not mine
and I haven't talked to CRIU people
about that.


> Using F_UNLCK for this is a bit kludgey, but adding a new
> constant is probably worse.
>
> I'm willing to take this in with an eye toward v6.6. Are you also
> willing to draft up some manpage patches that detail this new interface?
Sure thing.
As soon as its applied, I'll prepare a man
patch, or should it be done before that point?

2023-06-20 11:42:21

by Jeffrey Layton

[permalink] [raw]
Subject: Re: [PATCH 1/3] fs/locks: F_UNLCK extension for F_OFD_GETLK

On Tue, 2023-06-20 at 16:00 +0500, stsp wrote:
> Hello,
>
> 20.06.2023 15:46, Jeff Layton пишет:
> > On Tue, 2023-06-20 at 14:55 +0500, Stas Sergeev wrote:
> > > Currently F_UNLCK with F_OFD_GETLK returns -EINVAL.
> > > The proposed extension allows to use it for getting the lock
> > > information from the particular fd.
> > >
> > > Signed-off-by: Stas Sergeev <[email protected]>
> > >
> > > CC: Jeff Layton <[email protected]>
> > > CC: Chuck Lever <[email protected]>
> > > CC: Alexander Viro <[email protected]>
> > > CC: Christian Brauner <[email protected]>
> > > CC: [email protected]
> > > CC: [email protected]
> > >
> > > ---
> > > fs/locks.c | 23 ++++++++++++++++++++---
> > > 1 file changed, 20 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/fs/locks.c b/fs/locks.c
> > > index df8b26a42524..210766007e63 100644
> > > --- a/fs/locks.c
> > > +++ b/fs/locks.c
> > > @@ -868,6 +868,21 @@ static bool posix_locks_conflict(struct file_lock *caller_fl,
> > > return locks_conflict(caller_fl, sys_fl);
> > > }
> > >
> > > +/* Determine if lock sys_fl blocks lock caller_fl. Used on xx_GETLK
> > > + * path so checks for additional GETLK-specific things like F_UNLCK.
> > > + */
> > > +static bool posix_test_locks_conflict(struct file_lock *caller_fl,
> > > + struct file_lock *sys_fl)
> > > +{
> > > + /* F_UNLCK checks any locks on the same fd. */
> > > + if (caller_fl->fl_type == F_UNLCK) {
> > > + if (!posix_same_owner(caller_fl, sys_fl))
> > > + return false;
> > > + return locks_overlap(caller_fl, sys_fl);
> > > + }
> > > + return posix_locks_conflict(caller_fl, sys_fl);
> > > +}
> > > +
> > > /* Determine if lock sys_fl blocks lock caller_fl. FLOCK specific
> > > * checking before calling the locks_conflict().
> > > */
> > > @@ -901,7 +916,7 @@ posix_test_lock(struct file *filp, struct file_lock *fl)
> > > retry:
> > > spin_lock(&ctx->flc_lock);
> > > list_for_each_entry(cfl, &ctx->flc_posix, fl_list) {
> > > - if (!posix_locks_conflict(fl, cfl))
> > > + if (!posix_test_locks_conflict(fl, cfl))
> > > continue;
> > > if (cfl->fl_lmops && cfl->fl_lmops->lm_lock_expirable
> > > && (*cfl->fl_lmops->lm_lock_expirable)(cfl)) {
> > > @@ -2207,7 +2222,8 @@ int fcntl_getlk(struct file *filp, unsigned int cmd, struct flock *flock)
> > > if (fl == NULL)
> > > return -ENOMEM;
> > > error = -EINVAL;
> > > - if (flock->l_type != F_RDLCK && flock->l_type != F_WRLCK)
> > > + if (cmd != F_OFD_GETLK && flock->l_type != F_RDLCK
> > > + && flock->l_type != F_WRLCK)
> > > goto out;
> > >
> > > error = flock_to_posix_lock(filp, fl, flock);
> > > @@ -2414,7 +2430,8 @@ int fcntl_getlk64(struct file *filp, unsigned int cmd, struct flock64 *flock)
> > > return -ENOMEM;
> > >
> > > error = -EINVAL;
> > > - if (flock->l_type != F_RDLCK && flock->l_type != F_WRLCK)
> > > + if (cmd != F_OFD_GETLK && flock->l_type != F_RDLCK
> > > + && flock->l_type != F_WRLCK)
> > > goto out;
> > >
> > > error = flock64_to_posix_lock(filp, fl, flock);
> > This seems like a reasonable sort of interface to add, particularly for
> > the CRIU case.
>
> Just for the record: my own cases are
> the remaining 2. CRIU case is not mine
> and I haven't talked to CRIU people
> about that.
>
>
> > Using F_UNLCK for this is a bit kludgey, but adding a new
> > constant is probably worse.
> >
> > I'm willing to take this in with an eye toward v6.6. Are you also
> > willing to draft up some manpage patches that detail this new interface?
> Sure thing.
> As soon as its applied, I'll prepare a man
> patch, or should it be done before that point?

These days, it's a good idea to go ahead and draft that up early. You'll
be surprised what sort of details you notice once you have to start
writing documentation. ;)

You can post it as part of this set on the next posting and just mention
that it's a draft manpage patch. You should also include the linux-api
mailing list on the next posting so we get some feedback on the
interface itself.
--
Jeff Layton <[email protected]>

2023-06-21 16:10:37

by stsp

[permalink] [raw]
Subject: Re: [PATCH 1/3] fs/locks: F_UNLCK extension for F_OFD_GETLK


20.06.2023 16:15, Jeff Layton пишет:
> These days, it's a good idea to go ahead and draft that up early. You'll
> be surprised what sort of details you notice once you have to start
> writing documentation. ;)
>
> You can post it as part of this set on the next posting and just mention
> that it's a draft manpage patch. You should also include the linux-api
> mailing list on the next posting so we get some feedback on the
> interface itself.
v2 is sent with the proposed man page
update and a drop of an l_pid.
Thanks.