On Mon, 2021-06-14 at 11:04 +0100, David Howells wrote: > Jeff Layton <jlayton@xxxxxxxxxx> wrote: > > > +/** > > + * prep_noread_page - prep a page for writing without reading first > > It's a static function, so I'm not sure it needs the kernel doc marker. > > It also needs prefixing with "netfs_". > I added the comment since the logic here is somewhat complex. It didn't need to be a kerneldoc header, but I figured that didn't hurt anything. > > + /* pos beyond last page in the file */ > > + if (index > ((i_size - 1) / thp_size(page))) > > + goto zero_out; > > thp_size() is not a constant, so this gets you a DIV instruction. > Ugh, ok. > Why not: > > if (page_offset(page) >= i_size) > That doesn't handle THP's correctly. It's just a PAGE_SIZE shift. > or maybe: > > if (pos - offset >= i_size) > That might work. > > + zero_user_segments(page, 0, offset, offset + len, thp_size(page)); > > If you're going to leave a hole in the file, this will break afs, so this > patch needs to deal with that too (basically if copied < len, then the > remainder needs clearing, give or take len being trimmed to the end of the > page). I can look at adding that. > I think we have to contend with that in write_end. Basically if the copy is short, then we probably want to pretend it was a zero length copy and let generic_perform_write handle it as such. See commit b9de313cf05fe where Al fixed some sketchy error handling in ceph_write_end along those lines. > Matthew Wilcox <willy@xxxxxxxxxxxxx> wrote: > > > > + size_t offset = offset_in_page(pos); > > > > offset_in_thp(page, pos); > > I can make this change too. > Thanks. > (btw, can offset_in_thp() have it's second arg renamed to 'pos', not just 'p'? > 'p' is normally used to indicate a pointer of some sort). > -- Jeff Layton <jlayton@xxxxxxxxxx>
