2011/11/8 Sachin Prabhu <sprabhu@xxxxxxxxxx>:
> On Tue, 2011-11-08 at 11:57 +0400, Pavel Shilovsky wrote:
>> 2011/11/8 Sachin Prabhu <sprabhu@xxxxxxxxxx>:
>> > Allow flock over a cifs mount.
>> >
>> > Signed-off-by: Sachin Prabhu <sprabhu@xxxxxxxxxx>
>> >
>> > diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c
>> > index 8f1fe32..56d3b36 100644
>> > --- a/fs/cifs/cifsfs.c
>> > +++ b/fs/cifs/cifsfs.c
>> > @@ -830,6 +830,7 @@ const struct file_operations cifs_file_ops = {
>> > .open = cifs_open,
>> > .release = cifs_close,
>> > .lock = cifs_lock,
>> > + .flock = cifs_flock,
>> > .fsync = cifs_fsync,
>> > .flush = cifs_flush,
>> > .mmap = cifs_file_mmap,
>> > @@ -849,6 +850,7 @@ const struct file_operations cifs_file_strict_ops = {
>> > .open = cifs_open,
>> > .release = cifs_close,
>> > .lock = cifs_lock,
>> > + .flock = cifs_flock,
>> > .fsync = cifs_strict_fsync,
>> > .flush = cifs_flush,
>> > .mmap = cifs_file_strict_mmap,
>> > @@ -869,6 +871,7 @@ const struct file_operations cifs_file_direct_ops = {
>> > .open = cifs_open,
>> > .release = cifs_close,
>> > .lock = cifs_lock,
>> > + .flock = cifs_flock,
>> > .fsync = cifs_fsync,
>> > .flush = cifs_flush,
>> > .mmap = cifs_file_mmap,
>> > diff --git a/fs/cifs/cifsfs.h b/fs/cifs/cifsfs.h
>> > index 30ff560..e119e9d 100644
>> > --- a/fs/cifs/cifsfs.h
>> > +++ b/fs/cifs/cifsfs.h
>> > @@ -87,6 +87,7 @@ extern ssize_t cifs_user_writev(struct kiocb *iocb, const struct iovec *iov,
>> > extern ssize_t cifs_strict_writev(struct kiocb *iocb, const struct iovec *iov,
>> > unsigned long nr_segs, loff_t pos);
>> > extern int cifs_lock(struct file *, int, struct file_lock *);
>> > +extern int cifs_flock(struct file *, int, struct file_lock *);
>> > extern int cifs_fsync(struct file *, loff_t, loff_t, int);
>> > extern int cifs_strict_fsync(struct file *, loff_t, loff_t, int);
>> > extern int cifs_flush(struct file *, fl_owner_t id);
>> > diff --git a/fs/cifs/file.c b/fs/cifs/file.c
>> > index c1f063c..dd34f0b 100644
>> > --- a/fs/cifs/file.c
>> > +++ b/fs/cifs/file.c
>> > @@ -43,6 +43,23 @@
>> > #include "cifs_fs_sb.h"
>> > #include "fscache.h"
>> >
>> > +static int do_vfs_lock(struct file *file, struct file_lock *fl)
>> > +{
>> > + int res = 0;
>> > +
>> > + switch (fl->fl_flags & (FL_POSIX|FL_FLOCK)) {
>> > + case FL_POSIX:
>> > + res = posix_lock_file_wait(file, fl);
>> > + break;
>> > + case FL_FLOCK:
>> > + res = flock_lock_file_wait(file, fl);
>> > + break;
>> > + default:
>> > + BUG();
>> > + }
>> > + return res;
>> > +}
>> > +
>> > static inline int cifs_convert_flags(unsigned int flags)
>> > {
>> > if ((flags & O_ACCMODE) == O_RDONLY)
>> > @@ -814,7 +831,7 @@ cifs_posix_lock_set(struct file *file, struct file_lock *flock)
>> > struct cifsInodeInfo *cinode = CIFS_I(file->f_path.dentry->d_inode);
>> > int rc = 1;
>> >
>> > - if ((flock->fl_flags & FL_POSIX) == 0)
>> > + if ((flock->fl_flags & (FL_POSIX | FL_FLOCK)) == 0)
>> > return rc;
>> >
>> > mutex_lock(&cinode->lock_mutex);
>> > @@ -822,7 +839,7 @@ cifs_posix_lock_set(struct file *file, struct file_lock *flock)
>> > mutex_unlock(&cinode->lock_mutex);
>> > return rc;
>> > }
>> > - rc = posix_lock_file_wait(file, flock);
>> > + rc = do_vfs_lock(file, flock);
>> > mutex_unlock(&cinode->lock_mutex);
>> > return rc;
>> > }
>> > @@ -1231,12 +1248,27 @@ cifs_setlk(struct file *file, struct file_lock *flock, __u8 type,
>> > struct cifs_tcon *tcon = tlink_tcon(cfile->tlink);
>> > struct cifsInodeInfo *cinode = CIFS_I(file->f_path.dentry->d_inode);
>> > __u16 netfid = cfile->netfid;
>> > + unsigned char fl_flags = flock->fl_flags;
>> > +
>> > + if (unlock) {
>> > + /*Check for existance of lock*/
>> > + flock->fl_flags |= FL_EXISTS;
>> > + if (do_vfs_lock(flock->fl_file, flock) == -ENOENT)
>> > + goto out;
>> > + } else {
>> > + /*Check if we can obtain lock*/
>> > + flock->fl_flags |= FL_ACCESS;
>> > + rc = do_vfs_lock(flock->fl_file, flock);
>> > + if (rc < 0)
>> > + goto out;
>> > + }
>> > + flock->fl_flags = fl_flags;
>>
>> I think this should be skipped for non-posix locks. These locks have
>> their own cache (cifs_lock_* functions) associated with cifsInodeInfo
>> structure - so, we can't make a solution to 'goto out;' without
>> looking at this cache.
>
> I could follow Jeff's suggestion here and take the BUG out of
> do_vfs_lock(). In case of non posix/non flocks, the do_vfs_lock will
> simply return 0 and not trigger the goto out and proceed normally.
Sorry, I think I wasn't clear. In CIFS we have two locks variant
according to whether we use POSIX extensions or not. In the case of
POSIX extensions enabled we use VFS brlock cache and it is mirrors the
locks we have on the remote side. So, everything is good in this case.
Let's look at the case when POSIX extensions are disabled - Windows
locking style. The brlocks behavior changes and we use our own cache
for them but we need to call posix_file_lock_wait at the end of
cifs_setlk to make some use-cases work (e.g. unlock locks of the
child process when it finishes, etc) . And these locks are still
FL_POSIX locks from VFS point of view.
So, I am ok with your changes for POSIX extensions enabled case. Let's move
>> > + if (unlock) {
>> > + /*Check for existance of lock*/
>> > + flock->fl_flags |= FL_EXISTS;
>> > + if (do_vfs_lock(flock->fl_file, flock) == -ENOENT)
>> > + goto out;
>> > + } else {
>> > + /*Check if we can obtain lock*/
>> > + flock->fl_flags |= FL_ACCESS;
>> > + rc = do_vfs_lock(flock->fl_file, flock);
>> > + if (rc < 0)
>> > + goto out;
>> > + }
>> > + flock->fl_flags = fl_flags;
to the beginning of 'if (posix_lck) {}' and
> > + if (rc == 0 && lock) {
> > + /*We have to sleep here*/
> > + flock->fl_flags |= FL_SLEEP;
> > + do_vfs_lock(file, flock);
> > + }
> > + flock->fl_flags = fl_flags;
return rc;
to the end of it.
>
>
>>
>> >
>> > if (posix_lck) {
>> > int posix_lock_type;
>> >
>> > rc = cifs_posix_lock_set(file, flock);
>> > - if (!rc || rc < 0)
>> > + if (rc <= 0)
>> > return rc;
>> >
>> > if (type & LOCKING_ANDX_SHARED_LOCK)
>> > @@ -1250,7 +1282,7 @@ cifs_setlk(struct file *file, struct file_lock *flock, __u8 type,
>> > rc = CIFSSMBPosixLock(xid, tcon, netfid, current->tgid,
>> > 0 /* set */, length, flock,
>> > posix_lock_type, wait_flag);
>> > - goto out;
>> > + goto out_lock;
>> > }
>> >
>> > if (lock) {
>> > @@ -1259,7 +1291,7 @@ cifs_setlk(struct file *file, struct file_lock *flock, __u8 type,
>> > if (rc < 0)
>> > return rc;
>> > else if (!rc)
>> > - goto out;
>> > + goto out_lock;
>> >
>> > rc = CIFSSMBLock(xid, tcon, netfid, current->tgid, length,
>> > flock->fl_start, 0, 1, type, wait_flag, 0);
>> > @@ -1271,9 +1303,14 @@ cifs_setlk(struct file *file, struct file_lock *flock, __u8 type,
>> > } else if (unlock)
>> > rc = cifs_unlock_range(cfile, flock, xid);
>> >
>> > +out_lock:
>> > + if (rc == 0 && lock) {
>> > + /*We have to sleep here*/
>> > + flock->fl_flags |= FL_SLEEP;
>>
>> It's not clear why we should sleep here - the patch description
>> doesn't tell anything about it.
>
> At this stage you have a lock on the remote server. You need to make
> sure you have a lock on the local machine before you proceed. This check
> is similar to the NFS client code.
>
>>
>> > + do_vfs_lock(file, flock);
>> > + }
>>
>> What's about unlock case here?
>
> Unlock is done in the lines above in the if(unlock) case. We set flag
> FL_EXISTS and then do a unlock. In case the lock to be unlocked doesn't
> exist, we error with a ENOENT and exit. If it does find a lock, it
> unlocks and then proceeds to unlock on the remote server too.
>
> This is required because the file close routine will call unlock calls
> for both posix and flock without actually determining what sort of lock
> is held. Without this code, my test module was making 2 unlock requests
> ie. one for posix locks and one for flocks for same same flock to the
> remote server.
>
>>
>> > out:
>> > - if (flock->fl_flags & FL_POSIX)
>> > - posix_lock_file_wait(file, flock);
And simply call do_vfs_lock() rather than posix_lock_file_wait() here
for Windows locking case.
>> > + flock->fl_flags = fl_flags;
>> > return rc;
>> > }
>> >
>> > @@ -1334,6 +1371,22 @@ int cifs_lock(struct file *file, int cmd, struct file_lock *flock)
>> > return rc;
>> > }
>> >
>> > +int cifs_flock(struct file *file, int cmd, struct file_lock *flock)
>> > +{
>> > + int rc;
>> > +
>> > + rc = cifs_lock(file, cmd, flock);
>> > +
>> > + /*
>> > + * flock requires -EWOULDBLOCK in case of conflicting locks
>> > + * and LOCK_NB is used
>> > + */
>> > + if ((rc == -EACCES) && !(flock->fl_flags & FL_SLEEP))
>> > + rc = -EWOULDBLOCK;
>> > +
>> > + return rc;
>> > +}
>> > +
>> > /* update the file size (if needed) after a write */
>> > void
>> > cifs_update_eof(struct cifsInodeInfo *cifsi, loff_t offset,
>> >
>> >
>> >
>>
>> In general, I think that this patch does much more than it's mentioned
>> in the description and brings a lot of changes in the existing lock
>> code.
>>
>> It seems like we should only change posix_file_lock_wait to your
>> do_vfs_lock in 'out' label and cifs_posix_lock_set for posix_lck case
>> and map flock to brlock from 0 to INF for !posix_lck case.
>>
>
> When coding this, I followed the NFS client code which adds more checks
> and balances to ensure that the local and remote locks do not go out of
> sync.
>
> Sachin Prabhu
>
>
--
Best regards,
Pavel Shilovsky.
--
To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
