[PATCH] ceph: don't allow setlease on cephfs

[PATCH] ceph: don't allow setlease on cephfs

[Jeff Layton]

Leases don't currently work correctly on kcephfs, as they are not broken
when caps are revoked. They could eventually be implemented similarly to
how we did them in libcephfs, but for now don't allow them.

Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
 fs/ceph/dir.c  | 2 ++
 fs/ceph/file.c | 1 +
 2 files changed, 3 insertions(+)

diff --git a/fs/ceph/dir.c b/fs/ceph/dir.c
index 040eaad9d063..34f669220a8b 100644
--- a/fs/ceph/dir.c
+++ b/fs/ceph/dir.c
@@ -1935,6 +1935,7 @@ const struct file_operations ceph_dir_fops = {
 	.compat_ioctl = compat_ptr_ioctl,
 	.fsync = ceph_fsync,
 	.lock = ceph_lock,
+	.setlease = simple_nosetlease,
 	.flock = ceph_flock,
 };
 
@@ -1943,6 +1944,7 @@ const struct file_operations ceph_snapdir_fops = {
 	.llseek = ceph_dir_llseek,
 	.open = ceph_open,
 	.release = ceph_release,
+	.setlease = simple_nosetlease,
 };
 
 const struct inode_operations ceph_dir_iops = {
diff --git a/fs/ceph/file.c b/fs/ceph/file.c
index 27c7047c9383..fb3ea715a19d 100644
--- a/fs/ceph/file.c
+++ b/fs/ceph/file.c
@@ -2505,6 +2505,7 @@ const struct file_operations ceph_file_fops = {
 	.mmap = ceph_mmap,
 	.fsync = ceph_fsync,
 	.lock = ceph_lock,
+	.setlease = simple_nosetlease,
 	.flock = ceph_flock,
 	.splice_read = generic_file_splice_read,
 	.splice_write = iter_file_splice_write,
-- 
2.26.2

Re: [PATCH] ceph: don't allow setlease on cephfs

[Ilya Dryomov]

On Thu, Aug 20, 2020 at 5:13 PM Jeff Layton <jlayton@kernel.org> wrote:
>
> Leases don't currently work correctly on kcephfs, as they are not broken
> when caps are revoked. They could eventually be implemented similarly to
> how we did them in libcephfs, but for now don't allow them.
>
> Signed-off-by: Jeff Layton <jlayton@kernel.org>
> ---
>  fs/ceph/dir.c  | 2 ++
>  fs/ceph/file.c | 1 +
>  2 files changed, 3 insertions(+)
>
> diff --git a/fs/ceph/dir.c b/fs/ceph/dir.c
> index 040eaad9d063..34f669220a8b 100644
> --- a/fs/ceph/dir.c
> +++ b/fs/ceph/dir.c
> @@ -1935,6 +1935,7 @@ const struct file_operations ceph_dir_fops = {
>         .compat_ioctl = compat_ptr_ioctl,
>         .fsync = ceph_fsync,
>         .lock = ceph_lock,
> +       .setlease = simple_nosetlease,
>         .flock = ceph_flock,
>  };
>
> @@ -1943,6 +1944,7 @@ const struct file_operations ceph_snapdir_fops = {
>         .llseek = ceph_dir_llseek,
>         .open = ceph_open,
>         .release = ceph_release,
> +       .setlease = simple_nosetlease,

Hi Jeff,

Isn't this redundant for directories?

Thanks,

                Ilya

Re: [PATCH] ceph: don't allow setlease on cephfs

[Jeff Layton]

On Mon, 2020-08-24 at 17:38 +0200, Ilya Dryomov wrote:
> On Thu, Aug 20, 2020 at 5:13 PM Jeff Layton <jlayton@kernel.org> wrote:
> > Leases don't currently work correctly on kcephfs, as they are not broken
> > when caps are revoked. They could eventually be implemented similarly to
> > how we did them in libcephfs, but for now don't allow them.
> > 
> > Signed-off-by: Jeff Layton <jlayton@kernel.org>
> > ---
> >  fs/ceph/dir.c  | 2 ++
> >  fs/ceph/file.c | 1 +
> >  2 files changed, 3 insertions(+)
> > 
> > diff --git a/fs/ceph/dir.c b/fs/ceph/dir.c
> > index 040eaad9d063..34f669220a8b 100644
> > --- a/fs/ceph/dir.c
> > +++ b/fs/ceph/dir.c
> > @@ -1935,6 +1935,7 @@ const struct file_operations ceph_dir_fops = {
> >         .compat_ioctl = compat_ptr_ioctl,
> >         .fsync = ceph_fsync,
> >         .lock = ceph_lock,
> > +       .setlease = simple_nosetlease,
> >         .flock = ceph_flock,
> >  };
> > 
> > @@ -1943,6 +1944,7 @@ const struct file_operations ceph_snapdir_fops = {
> >         .llseek = ceph_dir_llseek,
> >         .open = ceph_open,
> >         .release = ceph_release,
> > +       .setlease = simple_nosetlease,
> 
> Hi Jeff,
> 
> Isn't this redundant for directories?
> 
> Thanks,
> 
>                 Ilya

generic_setlease does currently return -EINVAL if you try to set it on
anything but a regular file. But, there is nothing that prevents that at
a higher level. A filesystem can implement a ->setlease op that allows
it.

So yeah, that doesn't really have of an effect since you'd likely get
back -EINVAL anyway, but adding this line in it makes that explicit.
-- 
Jeff Layton <jlayton@kernel.org>

Re: [PATCH] ceph: don't allow setlease on cephfs

[Ilya Dryomov]

On Mon, Aug 24, 2020 at 6:03 PM Jeff Layton <jlayton@kernel.org> wrote:
>
> On Mon, 2020-08-24 at 17:38 +0200, Ilya Dryomov wrote:
> > On Thu, Aug 20, 2020 at 5:13 PM Jeff Layton <jlayton@kernel.org> wrote:
> > > Leases don't currently work correctly on kcephfs, as they are not broken
> > > when caps are revoked. They could eventually be implemented similarly to
> > > how we did them in libcephfs, but for now don't allow them.
> > >
> > > Signed-off-by: Jeff Layton <jlayton@kernel.org>
> > > ---
> > >  fs/ceph/dir.c  | 2 ++
> > >  fs/ceph/file.c | 1 +
> > >  2 files changed, 3 insertions(+)
> > >
> > > diff --git a/fs/ceph/dir.c b/fs/ceph/dir.c
> > > index 040eaad9d063..34f669220a8b 100644
> > > --- a/fs/ceph/dir.c
> > > +++ b/fs/ceph/dir.c
> > > @@ -1935,6 +1935,7 @@ const struct file_operations ceph_dir_fops = {
> > >         .compat_ioctl = compat_ptr_ioctl,
> > >         .fsync = ceph_fsync,
> > >         .lock = ceph_lock,
> > > +       .setlease = simple_nosetlease,
> > >         .flock = ceph_flock,
> > >  };
> > >
> > > @@ -1943,6 +1944,7 @@ const struct file_operations ceph_snapdir_fops = {
> > >         .llseek = ceph_dir_llseek,
> > >         .open = ceph_open,
> > >         .release = ceph_release,
> > > +       .setlease = simple_nosetlease,
> >
> > Hi Jeff,
> >
> > Isn't this redundant for directories?
> >
> > Thanks,
> >
> >                 Ilya
>
> generic_setlease does currently return -EINVAL if you try to set it on
> anything but a regular file. But, there is nothing that prevents that at
> a higher level. A filesystem can implement a ->setlease op that allows
> it.
>
> So yeah, that doesn't really have of an effect since you'd likely get
> back -EINVAL anyway, but adding this line in it makes that explicit.

It looks like gfs2 and nfs only set simple_nosetlease for file fops,
so it might be more consistent if we did this only for ceph_file_fops
as well.

Thanks,

                Ilya

Re: [PATCH] ceph: don't allow setlease on cephfs

[Jeff Layton]

On Mon, 2020-08-24 at 18:35 +0200, Ilya Dryomov wrote:
> On Mon, Aug 24, 2020 at 6:03 PM Jeff Layton <jlayton@kernel.org> wrote:
> > On Mon, 2020-08-24 at 17:38 +0200, Ilya Dryomov wrote:
> > > On Thu, Aug 20, 2020 at 5:13 PM Jeff Layton <jlayton@kernel.org> wrote:
> > > > Leases don't currently work correctly on kcephfs, as they are not broken
> > > > when caps are revoked. They could eventually be implemented similarly to
> > > > how we did them in libcephfs, but for now don't allow them.
> > > > 
> > > > Signed-off-by: Jeff Layton <jlayton@kernel.org>
> > > > ---
> > > >  fs/ceph/dir.c  | 2 ++
> > > >  fs/ceph/file.c | 1 +
> > > >  2 files changed, 3 insertions(+)
> > > > 
> > > > diff --git a/fs/ceph/dir.c b/fs/ceph/dir.c
> > > > index 040eaad9d063..34f669220a8b 100644
> > > > --- a/fs/ceph/dir.c
> > > > +++ b/fs/ceph/dir.c
> > > > @@ -1935,6 +1935,7 @@ const struct file_operations ceph_dir_fops = {
> > > >         .compat_ioctl = compat_ptr_ioctl,
> > > >         .fsync = ceph_fsync,
> > > >         .lock = ceph_lock,
> > > > +       .setlease = simple_nosetlease,
> > > >         .flock = ceph_flock,
> > > >  };
> > > > 
> > > > @@ -1943,6 +1944,7 @@ const struct file_operations ceph_snapdir_fops = {
> > > >         .llseek = ceph_dir_llseek,
> > > >         .open = ceph_open,
> > > >         .release = ceph_release,
> > > > +       .setlease = simple_nosetlease,
> > > 
> > > Hi Jeff,
> > > 
> > > Isn't this redundant for directories?
> > > 
> > > Thanks,
> > > 
> > >                 Ilya
> > 
> > generic_setlease does currently return -EINVAL if you try to set it on
> > anything but a regular file. But, there is nothing that prevents that at
> > a higher level. A filesystem can implement a ->setlease op that allows
> > it.
> > 
> > So yeah, that doesn't really have of an effect since you'd likely get
> > back -EINVAL anyway, but adding this line in it makes that explicit.
> 
> It looks like gfs2 and nfs only set simple_nosetlease for file fops,
> so it might be more consistent if we did this only for ceph_file_fops
> as well.
> 

Fair enough. Do you want to just fix it up, or would you rather I send a v2?

Thanks,
-- 
Jeff Layton <jlayton@kernel.org>

Re: [PATCH] ceph: don't allow setlease on cephfs

[Ilya Dryomov]

On Mon, Aug 24, 2020 at 6:37 PM Jeff Layton <jlayton@kernel.org> wrote:
>
> On Mon, 2020-08-24 at 18:35 +0200, Ilya Dryomov wrote:
> > On Mon, Aug 24, 2020 at 6:03 PM Jeff Layton <jlayton@kernel.org> wrote:
> > > On Mon, 2020-08-24 at 17:38 +0200, Ilya Dryomov wrote:
> > > > On Thu, Aug 20, 2020 at 5:13 PM Jeff Layton <jlayton@kernel.org> wrote:
> > > > > Leases don't currently work correctly on kcephfs, as they are not broken
> > > > > when caps are revoked. They could eventually be implemented similarly to
> > > > > how we did them in libcephfs, but for now don't allow them.
> > > > >
> > > > > Signed-off-by: Jeff Layton <jlayton@kernel.org>
> > > > > ---
> > > > >  fs/ceph/dir.c  | 2 ++
> > > > >  fs/ceph/file.c | 1 +
> > > > >  2 files changed, 3 insertions(+)
> > > > >
> > > > > diff --git a/fs/ceph/dir.c b/fs/ceph/dir.c
> > > > > index 040eaad9d063..34f669220a8b 100644
> > > > > --- a/fs/ceph/dir.c
> > > > > +++ b/fs/ceph/dir.c
> > > > > @@ -1935,6 +1935,7 @@ const struct file_operations ceph_dir_fops = {
> > > > >         .compat_ioctl = compat_ptr_ioctl,
> > > > >         .fsync = ceph_fsync,
> > > > >         .lock = ceph_lock,
> > > > > +       .setlease = simple_nosetlease,
> > > > >         .flock = ceph_flock,
> > > > >  };
> > > > >
> > > > > @@ -1943,6 +1944,7 @@ const struct file_operations ceph_snapdir_fops = {
> > > > >         .llseek = ceph_dir_llseek,
> > > > >         .open = ceph_open,
> > > > >         .release = ceph_release,
> > > > > +       .setlease = simple_nosetlease,
> > > >
> > > > Hi Jeff,
> > > >
> > > > Isn't this redundant for directories?
> > > >
> > > > Thanks,
> > > >
> > > >                 Ilya
> > >
> > > generic_setlease does currently return -EINVAL if you try to set it on
> > > anything but a regular file. But, there is nothing that prevents that at
> > > a higher level. A filesystem can implement a ->setlease op that allows
> > > it.
> > >
> > > So yeah, that doesn't really have of an effect since you'd likely get
> > > back -EINVAL anyway, but adding this line in it makes that explicit.
> >
> > It looks like gfs2 and nfs only set simple_nosetlease for file fops,
> > so it might be more consistent if we did this only for ceph_file_fops
> > as well.
> >
>
> Fair enough. Do you want to just fix it up, or would you rather I send a v2?

I'll fix it up.

Thanks,

                Ilya