Stop treating size 0 as unknown size in vnode_create_vobject().

[pjd]

Summary

Whenever file is created, the vnode_create_vobject() function will
try to determine its size by calling vn_getsize_locked() as size 0
is ambigious: it means either the file size is 0 or the file size
is unknown.

Introduce special value for the size argument: VNODE_NO_SIZE.
Only when it is given, the vnode_create_vobject() will try to obtain
file's size on its own.

[kib]

sys/vm/vnode_pager.c

168 ↗

(On Diff #138722)

I do not understand this. The new code calls vn_getsize_locked() both for size == 0 and size == VNODE_NO_SIZE, while the stated goal is to avoid vn_getsize_locked() call for size == 0.

[pjd]

• Don't call vn_getsize_locked() in case of VNODE_NO_SIZE.
• Assume we can have a disk for both cases (size == 0 and size == VNODE_NO_SIZE).

[pjd]

Style.

[kib]

sys/vm/vnode_pager.c

156 ↗

(On Diff #138724)

You can reuse the vn_isdisk() check from there. E.g. this place might become

if (vn_isdisk(vp)) {
  if (size == 0 || isize == VNODE_NO_SIZE)
     size = IDX_TO_OFF(INT_MAX);
} else if (!vn_canvmio(vp)) {
    return (0);
}
and then vn_isdisk check below can be eliminated. vn_isdisk takes the devmtx.

BTW, can size == 0 for disks happen?  As I understand, this is to handle zero mediasize in g_vfs_open().  Is this possible?

[pjd]

sys/vm/vnode_pager.c

156 ↗

(On Diff #138724)

You can reuse the vn_isdisk() check from there. E.g. this place might become

if (vn_isdisk(vp)) {
  if (size == 0 || isize == VNODE_NO_SIZE)
     size = IDX_TO_OFF(INT_MAX);
} else if (!vn_canvmio(vp)) {
    return (0);
}
and then vn_isdisk check below can be eliminated. vn_isdisk takes the devmtx.

Maybe it would be possible to set some flag on the vnode and avoid devmtx altogether?

BTW, can size == 0 for disks happen? As I understand, this is to handle zero mediasize in g_vfs_open(). Is this possible?

From what I remember, you can have mediasize equals zero when there is no media, eg. you have a cd-rom without CD inside.

[pjd]

sys/vm/vnode_pager.c

156 ↗

(On Diff #138724)

BTW, can size == 0 for disks happen? As I understand, this is to handle zero mediasize in g_vfs_open(). Is this possible?

This is what happens when we try to open an empty cd0:

g_vfs_open:295: vp=0xfffff80006f52700 mediasize=0
vnode_create_vobject:160: vp=0xfffff80006f52700 size=0
g_vfs_done():cd0[READ(offset=65536, length=8192)]error = 6 supressing further ENXIO

I think we could just move the check for mediasize == 0 to g_vfs_open and return ENXIO there.

[pjd]

• Introduce dedicated vnode_disk_create_vobject() for use by g_vfs_open(), so we don't have to do vn_isdisk() checks for regular files.

• Handle the case of mediasize==0 in g_vfs_open().

[markj]

sys/geom/geom_vfs.c

295 ↗

(On Diff #138753)

Why do we call g_access() before this check? Is read access required in order to keep pp->mediasize stable?

sys/sys/vnode.h

1091 ↗

(On Diff #138753)

IMO, the name vnode_create_disk_vobject() keeps the namespace more consistent.

sys/vm/vnode_pager.c

189 ↗

(On Diff #138753)

KASSERT(isize == VNODE_NO_SIZE || isize >= 0)?

[pjd]

• Rename vnode_disk_create_vobject() to vnode_create_disk_vobject().
• Improve assertions.

[kib]

sys/vm/vnode_pager.c

189 ↗

(On Diff #138810)

Better use VNASSERT() there and in the next line.

That said, this assertion is too strong. Due to the devfs vnode lifecycle, the vnode lock is not enough to guarantee that vn_isdisk() returns true for disk.

[pjd]

sys/geom/geom_vfs.c

295 ↗

(On Diff #138753)

Why do we call g_access() before this check? Is read access required in order to keep pp->mediasize stable?

The mediasize property can be set on first access. See g_disk_access().

sys/vm/vnode_pager.c

189 ↗

(On Diff #138810)

That said, this assertion is too strong. Due to the devfs vnode lifecycle, the vnode lock is not enough to guarantee that vn_isdisk() returns true for disk.

Not sure if I understand this one. devfs doesn't create vm objects for vnodes. File systems goes to GEOM directly through g_vfs_* KPI.

[pjd]

Use VNASSERT() instead of KASSERT().

[pjd]

Add a missing argument to VNASSERT().

[kib]

sys/vm/vnode_pager.c

189 ↗

(On Diff #138810)

devfs (destroy_dev(9)) might cause the assert to become false at any time.

I misplaced the comment though: the issue is that vn_isdisk() for disk might become false, not this one.

[pjd]

Remove too aggressive assert.

[pjd]

sys/vm/vnode_pager.c

189 ↗

(On Diff #138810)

So basically when a process is trying to mount a file system while devfs entry is being destroyed? Makes sense. Thanks.

[olce]

Better indeed to return ENXIO early in g_vfs_open() and remove the special case for 0 in vnode_create_vobject(). Thanks.

[pjd]

Compilation fix.

[pjd]

Updated wrong review. Bring back the proper patch.

[pjd]

Add a comment explaining why we check mediasize this late.

[alc]

sys/vm/vnode_pager.c

188 ↗

(On Diff #138882)

This blank line can be deleted.

202 ↗

(On Diff #138882)

This blank line can be deleted.

[pjd]

Remove blank lines at the begining of functions with no local variables.

I see that style(9) has changed in this regard.

Requested by: alc

[allanjude]

approved by: allanjude (mentor)