Re: [PATCH v4] ceph: fix write_begin optimization when write is beyond EOF

[Jeff Layton <jlayton@kernel.org>]

On Sun, 2021-06-13 at 16:15 +0100, Matthew Wilcox wrote:
> On Sun, Jun 13, 2021 at 08:02:12AM -0400, Jeff Layton wrote:
> > > +	/* clamp length to end of the current page */
> > > +	if (len > PAGE_SIZE)
> > > +		len = PAGE_SIZE - offset;
> > 
> > Actually, I think this should be:
> > 
> > 	len = min(len, PAGE_SIZE - offset);
> > 
> > Otherwise, len could still go beyond the end of the page.
> 
> I don't understand why you want to clamp length instead of just coping
> with len being > PAGE_SIZE.
> 
> > > +
> > > +	/* full page write */
> > > +	if (offset == 0 && len == PAGE_SIZE)
> > > +		goto zero_out;
> 
> That becomes >=.
> 
> > > +	/* zero-length file */
> > > +	if (i_size == 0)
> > > +		goto zero_out;
> > > +
> > > +	/* position beyond last page in the file */
> > > +	if (index > ((i_size - 1) / PAGE_SIZE))
> > > +		goto zero_out;
> > > +
> > > +	/* write that covers the the page from start to EOF or beyond it */
> > > +	if (offset == 0 && (pos + len) >= i_size)
> > > +		goto zero_out;
> 
> That doesn't need any change.
> 
> > > +	return false;
> > > +zero_out:
> > > +	zero_user_segments(page, 0, offset, offset + len, PAGE_SIZE);
> 
> That also doesn't need any change.
> 

Won't it though? offset+len will could be beyond the end of the page at
that point. Hmm I guess zero_user_segments does this:

        if (start2 >= end2)
                start2 = end2 = 0;

...so that makes the second segment copy a no-op.

Ok, fair enough -- I'll get rid of the clamping and just allow len to be
longer than PAGE_SIZE in the checks.

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