Page MenuHomeFreeBSD

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

Authored by rmacklem on Jan 23 2021, 11:32 PM.

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
R10 FreeBSD src repository
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

rmacklem created this revision.

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.

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?

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).

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.