> On Aug 23, 2021, at 2:57 PM, J. Bruce Fields <bfields@xxxxxxxxxxxx> wrote:
>
> On Sat, Aug 21, 2021 at 04:30:43PM +0000, Chuck Lever III wrote:
>>> @@ -654,7 +658,12 @@ nlmsvc_unlock(struct net *net, struct nlm_file *file, struct nlm_lock *lock)
>>> nlmsvc_cancel_blocked(net, file, lock);
>>>
>>> lock->fl.fl_type = F_UNLCK;
>>> - error = vfs_lock_file(file->f_file, F_SETLK, &lock->fl, NULL);
>>> + if (file->f_file[0])
>>> + error = vfs_lock_file(file->f_file[0], F_SETLK,
>>> + &lock->fl, NULL);
>>> + if (file->f_file[1])
>>> + error = vfs_lock_file(file->f_file[1], F_SETLK,
>>> + &lock->fl, NULL);
>>
>> Eschew raw integers :-) Should the f_file array be indexed
>> using O_ flags as the comment below suggests?
>
> Sure, done.
>
>>> diff --git a/fs/lockd/svcsubs.c b/fs/lockd/svcsubs.c
>>> index f43d89e89c45..a0adaee245ae 100644
>>> --- a/fs/lockd/svcsubs.c
>>> +++ b/fs/lockd/svcsubs.c
>>> @@ -71,14 +71,38 @@ static inline unsigned int file_hash(struct nfs_fh *f)
>>> return tmp & (FILE_NRHASH - 1);
>>> }
>>>
>>> +int lock_to_openmode(struct file_lock *lock)
>>> +{
>>> + if (lock->fl_type == F_WRLCK)
>>> + return O_WRONLY;
>>> + else
>>> + return O_RDONLY;
>>
>> Style: a ternary would be more consistent with other areas
>> of the code you change in this patch.
>
> OK.
>
>>> +static int nlm_unlock_files(struct nlm_file *file)
>>> +{
>>> + struct file_lock lock;
>>> + struct file *f;
>>> +
>>> + lock.fl_type = F_UNLCK;
>>> + lock.fl_start = 0;
>>> + lock.fl_end = OFFSET_MAX;
>>> + for (f = file->f_file[0]; f <= file->f_file[1]; f++) {
>>> + if (f && vfs_lock_file(f, F_SETLK, &lock, NULL) < 0) {
>>> + printk("lockd: unlock failure in %s:%d\n",
>>> + __FILE__, __LINE__);
>>
>> This needs a KERN_LEVEL and maybe a _once.
>
> How about going with pr_warn for now.
Sold.
> I don't *think* there's much
> danger of spamming the logs from this one. And I'm wondering there
> might be some causes (unresponsive server?) that could resolve
> themselves, and then come back, and that you'd still want to hear about
> the second time.
>
>>> @@ -27,7 +27,8 @@ struct rpc_task;
>>> struct nlmsvc_binding {
>>> __be32 (*fopen)(struct svc_rqst *,
>>> struct nfs_fh *,
>>> - struct file **);
>>> + struct file **,
>>> + int mode);
>>
>> Style: "mode_t mode" might be better internal documentation.
>
> It's confusing that we use the word "mode" both for "access mode" (O_
> flags) and "mode bits" (permission bits). This is the former, and I
> thought mode_t was the for the latter.
Fair enough.
>
>>> @@ -154,7 +156,8 @@ struct nlm_rqst {
>>> struct nlm_file {
>>> struct hlist_node f_list; /* linked list */
>>> struct nfs_fh f_handle; /* NFS file handle */
>>> - struct file * f_file; /* VFS file pointer */
>>> + struct file * f_file[2]; /* VFS file pointers,
>>> + indexed by O_ flags */
>>
>> Right, except that the new code in this patch always indexes
>> f_file with raw integers, making the comment misleading. My
>> preference is to keep the comment and change the new code to
>> index f_file symbolically.
>
> OK.
>
> --b.
--
Chuck Lever
