2020-08-10 15:17:51

by David Howells

[permalink] [raw]
Subject: [GIT PULL] fscache rewrite -- please drop for now

Hi Linus,

Can you drop the fscache rewrite pull for now. We've seem an issue in NFS
integration and need to rework the read helper a bit. I made an assumption
that fscache will always be able to request that the netfs perform a read of a
certain minimum size - but with NFS you can break that by setting rsize too
small.

We need to make the read helper able to make multiple netfs reads. This can
help ceph too.

Thanks,
David


2020-08-10 15:37:11

by Steve French

[permalink] [raw]
Subject: Re: [GIT PULL] fscache rewrite -- please drop for now

cifs.ko also can set rsize quite small (even 1K for example, although
that will be more than 10x slower than the default 4MB so hopefully no
one is crazy enough to do that). I can't imagine an SMB3 server
negotiating an rsize or wsize smaller than 64K in today's world (and
typical is 1MB to 8MB) but the user can specify a much smaller rsize
on mount. If 64K is an adequate minimum, we could change the cifs
mount option parsing to require a certain minimum rsize if fscache is
selected.

On Mon, Aug 10, 2020 at 10:17 AM David Howells <[email protected]> wrote:
>
> Hi Linus,
>
> Can you drop the fscache rewrite pull for now. We've seem an issue in NFS
> integration and need to rework the read helper a bit. I made an assumption
> that fscache will always be able to request that the netfs perform a read of a
> certain minimum size - but with NFS you can break that by setting rsize too
> small.
>
> We need to make the read helper able to make multiple netfs reads. This can
> help ceph too.
>
> Thanks,
> David
>


--
Thanks,

Steve

2020-08-10 15:49:14

by David Howells

[permalink] [raw]
Subject: Re: [GIT PULL] fscache rewrite -- please drop for now

Steve French <[email protected]> wrote:

> cifs.ko also can set rsize quite small (even 1K for example, although
> that will be more than 10x slower than the default 4MB so hopefully no
> one is crazy enough to do that).

You can set rsize < PAGE_SIZE?

> I can't imagine an SMB3 server negotiating an rsize or wsize smaller than
> 64K in today's world (and typical is 1MB to 8MB) but the user can specify a
> much smaller rsize on mount. If 64K is an adequate minimum, we could change
> the cifs mount option parsing to require a certain minimum rsize if fscache
> is selected.

I've borrowed the 256K granule size used by various AFS implementations for
the moment. A 512-byte xattr can thus hold a bitmap covering 1G of file
space.

David

2020-08-10 16:37:15

by David Wysochanski

[permalink] [raw]
Subject: Re: [GIT PULL] fscache rewrite -- please drop for now

On Mon, Aug 10, 2020 at 11:48 AM David Howells <[email protected]> wrote:
>
> Steve French <[email protected]> wrote:
>
> > cifs.ko also can set rsize quite small (even 1K for example, although
> > that will be more than 10x slower than the default 4MB so hopefully no
> > one is crazy enough to do that).
>
> You can set rsize < PAGE_SIZE?
>
> > I can't imagine an SMB3 server negotiating an rsize or wsize smaller than
> > 64K in today's world (and typical is 1MB to 8MB) but the user can specify a
> > much smaller rsize on mount. If 64K is an adequate minimum, we could change
> > the cifs mount option parsing to require a certain minimum rsize if fscache
> > is selected.
>
> I've borrowed the 256K granule size used by various AFS implementations for
> the moment. A 512-byte xattr can thus hold a bitmap covering 1G of file
> space.
>
>

Is it possible to make the granule size configurable, then reject a
registration if the size is too small or not a power of 2? Then a
netfs using the API could try to set equal to rsize, and then error
out with a message if the registration was rejected.

2020-08-10 17:07:16

by Jeff Layton

[permalink] [raw]
Subject: Re: [GIT PULL] fscache rewrite -- please drop for now

On Mon, 2020-08-10 at 12:35 -0400, David Wysochanski wrote:
> On Mon, Aug 10, 2020 at 11:48 AM David Howells <[email protected]> wrote:
> > Steve French <[email protected]> wrote:
> >
> > > cifs.ko also can set rsize quite small (even 1K for example, although
> > > that will be more than 10x slower than the default 4MB so hopefully no
> > > one is crazy enough to do that).
> >
> > You can set rsize < PAGE_SIZE?
> >
> > > I can't imagine an SMB3 server negotiating an rsize or wsize smaller than
> > > 64K in today's world (and typical is 1MB to 8MB) but the user can specify a
> > > much smaller rsize on mount. If 64K is an adequate minimum, we could change
> > > the cifs mount option parsing to require a certain minimum rsize if fscache
> > > is selected.
> >
> > I've borrowed the 256K granule size used by various AFS implementations for
> > the moment. A 512-byte xattr can thus hold a bitmap covering 1G of file
> > space.
> >
> >
>
> Is it possible to make the granule size configurable, then reject a
> registration if the size is too small or not a power of 2? Then a
> netfs using the API could try to set equal to rsize, and then error
> out with a message if the registration was rejected.
>

...or maybe we should just make fscache incompatible with an
rsize that isn't an even multiple of 256k? You need to set mount options
for both, typically, so it would be fairly trivial to check this at
mount time, I'd think.

--
Jeff Layton <[email protected]>

2020-08-17 19:27:35

by Steven French

[permalink] [raw]
Subject: Re: [GIT PULL] fscache rewrite -- please drop for now


On 8/10/20 12:06 PM, Jeff Layton wrote:
> On Mon, 2020-08-10 at 12:35 -0400, David Wysochanski wrote:
>> On Mon, Aug 10, 2020 at 11:48 AM David Howells <[email protected]> wrote:
>>> Steve French <[email protected]> wrote:
>>>
>>>> cifs.ko also can set rsize quite small (even 1K for example, although
>>>> that will be more than 10x slower than the default 4MB so hopefully no
>>>> one is crazy enough to do that).
>>> You can set rsize < PAGE_SIZE?
>>>
>>>> I can't imagine an SMB3 server negotiating an rsize or wsize smaller than
>>>> 64K in today's world (and typical is 1MB to 8MB) but the user can specify a
>>>> much smaller rsize on mount. If 64K is an adequate minimum, we could change
>>>> the cifs mount option parsing to require a certain minimum rsize if fscache
>>>> is selected.
>>> I've borrowed the 256K granule size used by various AFS implementations for
>>> the moment. A 512-byte xattr can thus hold a bitmap covering 1G of file
>>> space.
>>>
>>>
>> Is it possible to make the granule size configurable, then reject a
>> registration if the size is too small or not a power of 2? Then a
>> netfs using the API could try to set equal to rsize, and then error
>> out with a message if the registration was rejected.
>>
> ...or maybe we should just make fscache incompatible with an
> rsize that isn't an even multiple of 256k? You need to set mount options
> for both, typically, so it would be fairly trivial to check this at
> mount time, I'd think.


Yes - if fscache is specified on mount it would be easy to round rsize
up (or down), at least for cifs.ko (perhaps simply in the mount.cifs
helper so a warning could be returned to the user) to whatever boundary
you prefer in fscache.   The default of 4MB (or 1MB for mounts to some
older servers) should be fine.  Similarly if the user requested the
default but the server negotiated an unusual size, not a multiple of
256K, we could round try to round it down if possible (or fail the mount
if not possible to round it down to 256K).

2020-08-27 15:30:41

by David Howells

[permalink] [raw]
Subject: Re: [GIT PULL] fscache rewrite -- please drop for now

Christoph Hellwig <[email protected]> wrote:

> FYI, a giant rewrite dropping support for existing consumer is always
> rather awkward. Is there any way you could pre-stage some infrastructure
> changes, and then do a temporary fscache2, which could then be renamed
> back to fscache once everyone switched over?

That's a bit tricky. There are three points that would have to be shared: the
userspace miscdev interface, the backing filesystem and the single index tree.

It's probably easier to just have a go at converting 9P and cifs. Making the
old and new APIs share would be a fairly hefty undertaking in its own right.

David

2020-08-27 16:20:46

by Dominique Martinet

[permalink] [raw]
Subject: Re: [GIT PULL] fscache rewrite -- please drop for now

David Howells wrote on Thu, Aug 27, 2020:
> Christoph Hellwig <[email protected]> wrote:
>
> > FYI, a giant rewrite dropping support for existing consumer is always
> > rather awkward. Is there any way you could pre-stage some infrastructure
> > changes, and then do a temporary fscache2, which could then be renamed
> > back to fscache once everyone switched over?
>
> That's a bit tricky. There are three points that would have to be shared: the
> userspace miscdev interface, the backing filesystem and the single index tree.
>
> It's probably easier to just have a go at converting 9P and cifs. Making the
> old and new APIs share would be a fairly hefty undertaking in its own right.

While I agree something incremental is probably better, I have some free
time over the next few weeks so will take a shot at 9p; it's definitely
going to be easier.


Should I submit patches to you or wait until Linus merges it next cycle
and send them directly?

I see Jeff's ceph patches are still in his tree's ceph-fscache-iter
branch and I don't see them anywhere in your tree.

--
Dominique

2020-08-27 17:15:47

by David Howells

[permalink] [raw]
Subject: Re: [GIT PULL] fscache rewrite -- please drop for now

Dominique Martinet <[email protected]> wrote:

> Should I submit patches to you or wait until Linus merges it next cycle
> and send them directly?
>
> I see Jeff's ceph patches are still in his tree's ceph-fscache-iter
> branch and I don't see them anywhere in your tree.

I really want them to all go in the same window, but there may be a
requirement for some filesystem-specific sets (eg. NFS) to go via the
maintainer tree.

Btw, at the moment, I'm looking at making the fscache read helper support the
new ->readahead() op.

David