Page MenuHomeFreeBSD
Differential D55665

p9fs: locking improvements for p9fs_stat_vnode_dotl()
AcceptedPublic
Actions

Authored by kib on Thu, Mar 5, 12:38 PM.
Tags
None
Referenced Files
F147356284: D55665.id.diff
Tue, Mar 10, 7:04 AM
F147349922: D55665.id173290.diff
Tue, Mar 10, 5:40 AM
F147244372: D55665.diff
Mon, Mar 9, 9:53 AM
F147238574: D55665.diff
Mon, Mar 9, 8:52 AM
F147237621: D55665.diff
Mon, Mar 9, 8:42 AM
Unknown Object (File)
Mon, Mar 9, 3:45 AM
Unknown Object (File)
Sun, Mar 8, 8:53 AM
Unknown Object (File)
Fri, Mar 6, 10:09 AM
View All 12 Files
Subscribers
arrowd
imp

Details

Reviewers
markj
Summary
If the vnode is share-locked:
- Use vn_delayed_setsize() to avoid calling vnode_pager_setsize() with
  the vnode only shared locked.
- Interlock the vnode to get exclusive mode for updating the node
  fields.
  
Reciprocally, interlock the vnode in p9fs_getattr_dotl() to observe the
consistent values on read.

PR:     293492

p9fs: use atomics for updating node->flags

This should prevent seeing inconsistent flags values when updating it
under the shared vnode lock.

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Skipped
Unit
Tests Skipped

Event Timeline

kib created this revision.Thu, Mar 5, 12:38 PM
Herald added a subscriber: imp. · View Herald TranscriptThu, Mar 5, 12:38 PM
kib requested review of this revision.Thu, Mar 5, 12:38 PM
kib added a parent revision: D55595: vn_delayed_setsize().
markj accepted this revision.Sat, Mar 7, 12:27 AM
markj added inline comments.
sys/fs/p9fs/p9fs_vnops.c
1004

Surely this and the assignments above should be exclusively locked? Maybe add an XXX comment for it.

This revision is now accepted and ready to land.Sat, Mar 7, 12:27 AM
kib marked an inline comment as done.Sat, Mar 7, 1:17 AM
kib added inline comments.
sys/fs/p9fs/p9fs_vnops.c
1004

I used the vnode interlock there, instead of adding the XXX comment.

kib updated this revision to Diff 173290.Sat, Mar 7, 1:18 AM
kib marked an inline comment as done.
kib retitled this revision from p9fs: use vn_delayed_setsize() to p9fs: locking improvements for p9fs_stat_vnode_dotl().
kib edited the summary of this revision. (Show Details)
This revision now requires review to proceed.Sat, Mar 7, 1:18 AM
markj added inline comments.Sun, Mar 8, 1:13 AM
sys/fs/p9fs/p9fs_vnops.c
973

I would not bother with the micro-optimization of avoiding the interlock. Oh, it's not an optimization, it's required to avoid a LOR with the VM object lock in vnode_pager_setsize(). I think a comment should explain this.

kib updated this revision to Diff 173324.Sun, Mar 8, 1:20 AM
kib marked an inline comment as done.

Add comment.

markj added inline comments.Sun, Mar 8, 1:27 AM
sys/fs/p9fs/p9fs_vnops.c
1000

p9fs_getattr_dotl() might still observe a torn write to these fields, since only the shared vnode lock will be held there, and the interlock is not acquired.

1004

Actually I think the exclusive vnode lock is required for this flags field, in the current scheme.

kib marked 2 inline comments as done.Sun, Mar 8, 4:47 AM
kib added inline comments.
sys/fs/p9fs/p9fs_vnops.c
1000

Yes, and it is enough to interlock on read too.

1004

IMO we can convert the flags updates to atomic.

kib updated this revision to Diff 173330.Sun, Mar 8, 4:48 AM
kib marked 2 inline comments as done.
kib edited the summary of this revision. (Show Details)

Use atomics for flags updates.
Interlock in getattr.

markj accepted this revision.Tue, Mar 10, 1:23 AM
This revision is now accepted and ready to land.Tue, Mar 10, 1:23 AM

Revision Contents
Changeset List

PathSize
sys/
fs/
p9fs/
40 lines

Diff 173324

View Options

sys/fs/p9fs/p9fs_vnops.c

Loading...