Page MenuHomeFreeBSD
Differential D28318

fix ncl_pager_setsize() so that the correct nsize can be set via a new argument
AbandonedPublic
Actions

Authored by rmacklem on Jan 23 2021, 11:32 PM.
Tags
None
Referenced Files
F147189276: D28318.diff
Sun, Mar 8, 11:35 PM
Unknown Object (File)
Sat, Mar 7, 10:47 AM
Unknown Object (File)
Sat, Mar 7, 10:30 AM
Unknown Object (File)
Mon, Mar 2, 1:29 AM
Unknown Object (File)
Mon, Feb 23, 3:07 AM
Unknown Object (File)
Mon, Feb 16, 9:28 PM
Unknown Object (File)
Mon, Feb 9, 6:33 PM
Unknown Object (File)
Sun, Feb 8, 5:41 AM
View All Files
Subscribers
cy

Details

Reviewers
kib
bdrewery
Summary

I think D28306 does not work because it sets the size
to va_size (n_vattr.na_size) and not n_size for the
write case.

This patch adds an "nsize" argument to ncl_pager_setsize()
so that the correct size can be passed in.
For the other calls, va_size/n_vattr.na_size is passed in,
so the semantics only changes for the write case added
by D28306.

Test Plan

This version works for the connectathon test suite,
whereas D28306 fails.
(I should have tested before clicking reviewed for
D28306.)

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Skipped
Unit
Tests Skipped

Event Timeline

rmacklem requested review of this revision.Jan 23 2021, 11:32 PM
rmacklem created this revision.
cy added a subscriber: cy.Jan 23 2021, 11:47 PM
kib added a comment.Jan 24 2021, 9:40 AM

Where are the changes to ncl_pager_setsize()?

I think that the good enough solution is what I did, i.e. rely on the vnode_pager_setsize() calls from the write loop. I just copied too much from nfs_read() which does not extend.

Or, to be absolutely pedantic, nfs_write() could set all sizes to something like max(np->n_size, np->n_vattr.na_vattr,va_size) to not undo the extension done by write.

rmacklem added a comment.Jan 24 2021, 3:32 PM

Comment inline.

sys/fs/nfsclient/nfs_clbio.c
1291

So, are you saying this change wasn't needed
and all that was required was the clearing of
NVNSETSZSKIP?

kib added inline comments.Jan 24 2021, 7:21 PM
sys/fs/nfsclient/nfs_clbio.c
1291

Well, I am saying that the minimal viable fix is what I did:

  1. Set TDP2_SBPAGES so that attrcache does not tried to shrink file while we busied some pages, to avoid self-deadlock.
  2. Clear NVNSETSZSKIP so that ncl_pager_setsize() did not triggered after TDP2_SBPAGES is cleared, unneeded, because we called vnode_pager_setsize() for write extents.

This is what my patch does.

As I said, it might be considered should we extend the file size when clearing TDP2_SBPAGES if attrcache decided to extend file beyond what writes extended (but definitely we want to ignore shrink).

rmacklem abandoned this revision.Jan 24 2021, 10:03 PM

Your patch sounds fine to me and is what I was
thinking when I asked.
(I didn't understand why the ncl_pager_setsize() call
was added, but I also admit I don't understand the
vm side of vnode_pager_setsze().)

I did test the case without ncl_pager_setsize() and it
seems to work for my test run.

I assume you are committing this, if you haven't already.

Revision Contents
Changeset List

PathSize
sys/
fs/
nfs/
nfsport.h
nfsport.h.orig
2 lines
nfsclient/
nfs_clbio.c
nfs_clbio.c.orig
16 lines
nfs_clport.c
nfs_clport.c.orig
2 lines
nfs_clsubs.c
nfs_clsubs.c.orig
2 lines

Diff 82802

View Options

sys/fs/nfs/nfsport.h

Loading...
View Options

sys/fs/nfsclient/nfs_clbio.c

Loading...
View Options

sys/fs/nfsclient/nfs_clport.c

Loading...
View Options

sys/fs/nfsclient/nfs_clsubs.c

Loading...