On Wed, Oct 22, 2008 at 05:47:46PM -0400, J. Bruce Fields wrote:
> On Wed, Oct 22, 2008 at 03:31:06PM -0500, Steve French wrote:
> > Oplock is a guarantee that no one else is changing (or reading) a
> > file. The notification that a file is now changing is different (that
> > is SMB change notify), Oplock is a guarantee that you are the only
> > writer (or reader/writer) of the file - so if the server wants to
> > revoke that (oplock break) - and with the patch we call break_lease
> > and don't return on the oplock break until break_lease returns, and
> > __break_lease in fs/locks.c looks synchronous - break_lease async?
>
> Ah! Yes, you're right. nfsd always calls break_lease with
> O_NONBLOCK....
>
> Makes sense to me, then.
>
> > By the way, the server also gives up on the client if it does not
> > respond (but this is a long time - more than 20 seconds for most
> > servers) - presumably by flushing the dirty pages, the issues the
> > oplock break response.
>
> So it's a little irritating that the server's timeout may not agree with
> the local timeout set by that sysctl. I suppose the lease timeout
> should really be a characteristic of the filesystem rather than a global
> thing.
More annoying: if there's a local lease, and someone local breaks it,
then I think they're going to block in break_lease() up to the time
given by that sysctl, before they even call into the filesystem.
So a little better coordination with the filesystem is going to be
needed before we get lease breaking completely right.
--b.
>
> --b.
>
> >
> > Following is an updated patch to add support for mount option to
> > disable the check for oplock (since not all server support oplock):
> >
> > diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c
> > index c6aad77..76919c2 100644
> > --- a/fs/cifs/cifsfs.c
> > +++ b/fs/cifs/cifsfs.c
> > @@ -618,6 +618,37 @@ static loff_t cifs_llseek(struct file *file,
> > loff_t offset, int origin)
> > return generic_file_llseek_unlocked(file, offset, origin);
> > }
> >
> > +#ifdef CONFIG_CIFS_EXPERIMENTAL
> > +static int cifs_setlease(struct file *file, long arg, struct file_lock **lease)
> > +{
> > + /* note that this is called by vfs setlease with the BKL held
> > + although I doubt that BKL is needed here in cifs */
> > + struct inode *inode = file->f_path.dentry->d_inode;
> > +
> > + if (!(S_ISREG(inode->i_mode)))
> > + return -EINVAL;
> > +
> > + /* check if file is oplocked */
> > + if (((arg == F_RDLCK) &&
> > + (CIFS_I(inode)->clientCanCacheRead)) ||
> > + ((arg == F_WRLCK) &&
> > + (CIFS_I(inode)->clientCanCacheAll)))
> > + return generic_setlease(file, arg, lease);
> > + else if (CIFS_SB(inode->i_sb)->tcon->local_lease &&
> > + !CIFS_I(inode)->clientCanCacheRead)
> > + /* If the server claims to support oplock on this
> > + file, then we still need to check oplock even
> > + if the local_lease mount option is set, but there
> > + are servers which do not support oplock for which
> > + this mount option may be useful if the user
> > + knows that the file won't be changed on the server
> > + by anyone else */
> > + return generic_setlease(file, arg, lease);
> > + else
> > + return -EAGAIN;
> > +}
> > +#endif
> > +
> > struct file_system_type cifs_fs_type = {
> > .owner = THIS_MODULE,
> > .name = "cifs",
> > @@ -696,6 +727,7 @@ const struct file_operations cifs_file_ops = {
> >
> > #ifdef CONFIG_CIFS_EXPERIMENTAL
> > .dir_notify = cifs_dir_notify,
> > + .setlease = cifs_setlease,
> > #endif /* CONFIG_CIFS_EXPERIMENTAL */
> > };
> >
> > @@ -716,6 +748,7 @@ const struct file_operations cifs_file_direct_ops = {
> > .llseek = cifs_llseek,
> > #ifdef CONFIG_CIFS_EXPERIMENTAL
> > .dir_notify = cifs_dir_notify,
> > + .setlease = cifs_setlease,
> > #endif /* CONFIG_CIFS_EXPERIMENTAL */
> > };
> > const struct file_operations cifs_file_nobrl_ops = {
> > @@ -736,6 +769,7 @@ const struct file_operations cifs_file_nobrl_ops = {
> >
> > #ifdef CONFIG_CIFS_EXPERIMENTAL
> > .dir_notify = cifs_dir_notify,
> > + .setlease = cifs_setlease,
> > #endif /* CONFIG_CIFS_EXPERIMENTAL */
> > };
> >
> > @@ -755,6 +789,7 @@ const struct file_operations cifs_file_direct_nobrl_ops = {
> > .llseek = cifs_llseek,
> > #ifdef CONFIG_CIFS_EXPERIMENTAL
> > .dir_notify = cifs_dir_notify,
> > + .setlease = cifs_setlease,
> > #endif /* CONFIG_CIFS_EXPERIMENTAL */
> > };
> >
> > @@ -946,6 +981,12 @@ static int cifs_oplock_thread(void *dummyarg)
> > the call */
> > /* mutex_lock(&inode->i_mutex);*/
> > if (S_ISREG(inode->i_mode)) {
> > +#ifdef CONFIG_CIFS_EXPERIMENTAL
> > + if (CIFS_I(inode)->clientCanCacheAll == 0)
> > + break_lease(inode, FMODE_READ);
> > + else if (CIFS_I(inode)->clientCanCacheRead == 0)
> > + break_lease(inode, FMODE_WRITE);
> > +#endif
> > rc = filemap_fdatawrite(inode->i_mapping);
> > if (CIFS_I(inode)->clientCanCacheRead == 0) {
> > waitrc = filemap_fdatawait(
> > diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
> > index 178f733..c791e5b 100644
> > --- a/fs/cifs/cifsglob.h
> > +++ b/fs/cifs/cifsglob.h
> > @@ -285,6 +285,7 @@ struct cifsTconInfo {
> > bool seal:1; /* transport encryption for this mounted share */
> > bool unix_ext:1; /* if false disable Linux extensions to CIFS protocol
> > for this mount even if server would support */
> > + bool local_lease:1; /* check leases (only) on local system not remote */
> > /* BB add field for back pointer to sb struct(s)? */
> > };
> >
> > diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
> > index 1126f7a..17058c5 100644
> > --- a/fs/cifs/connect.c
> > +++ b/fs/cifs/connect.c
> > @@ -90,7 +90,8 @@ struct smb_vol {
> > bool nocase:1; /* request case insensitive filenames */
> > bool nobrl:1; /* disable sending byte range locks to srv */
> > bool seal:1; /* request transport encryption on share */
> > - bool nodfs:1;
> > + bool nodfs:1; /* Do not request DFS, even if available */
> > + bool local_lease:1; /* check leases only on local system, not remote */
> > unsigned int rsize;
> > unsigned int wsize;
> > unsigned int sockopt;
> > @@ -1264,6 +1265,8 @@ cifs_parse_mount_options(char *options, const
> > char *devname,
> > vol->no_psx_acl = 0;
> > } else if (strnicmp(data, "noacl", 5) == 0) {
> > vol->no_psx_acl = 1;
> > + } else if (strnicmp(data, "locallease", 6) == 0) {
> > + vol->local_lease = 1;
> > } else if (strnicmp(data, "sign", 4) == 0) {
> > vol->secFlg |= CIFSSEC_MUST_SIGN;
> > } else if (strnicmp(data, "seal", 4) == 0) {
> > @@ -2162,6 +2165,7 @@ cifs_mount(struct super_block *sb, struct
> > cifs_sb_info *cifs_sb,
> > for the retry flag is used */
> > tcon->retry = volume_info.retry;
> > tcon->nocase = volume_info.nocase;
> > + tcon->local_lease = volume_info.local_lease;
> > if (tcon->seal != volume_info.seal)
> > cERROR(1, ("transport encryption setting "
> > "conflicts with existing tid"));
> >
> > On Wed, Oct 22, 2008 at 3:19 PM, J. Bruce Fields <bfields@xxxxxxxxxxxx> wrote:
> > > What guarantees that no other client can modify the file as long as the
> > > lease is held?
> > >
> > > It's not enough to break the lease as soon as you notice the file
> > > changes--the holder of the lease has up to 45 seconds (configurable with
> > > /proc/sys/fs/lease-break-time) to return the lease before the kernel
> > > forcibly revokes it.
> > >
> > > --b.
> >
> >
> >
> > --
> > Thanks,
> >
> > Steve
> --
> To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
