Re: ffs_truncate: Missing uvm_vnp_uncache() w/ softdep
On 24/05/22(Tue) 15:24, Mark Kettenis wrote:
> > Date: Tue, 24 May 2022 14:28:39 +0200
> > From: Martin Pieuchot
> >
> > The softdep code path is missing a UVM cache invalidation compared to
> > the !softdep one. This is necessary to flush pages of a persisting
> > vnode.
> >
> > Since uvm_vnp_setsize() is also called later in this function for the
> > !softdep case move it to not call it twice.
> >
> > ok?
>
> I'm not sure this is correct. I'm trying to understand why you're
> moving the uvm_uvn_setsize() call. Are you just trying to call it
> twice? Or are you trying to avoid calling it at all when we end up in
> an error path?
>
> The way you moved it means we'll still call it twice for "partially
> truncated" files with softdeps. At least the way I understand the
> code is that the code will fsync the vnode and dropping down in the
> "normal" non-softdep code that will call uvm_vnp_setsize() (and
> uvn_vnp_uncache()) again. So maybe you should move the
> uvm_uvn_setsize() call into the else case?
We might want to do that indeed. I'm not sure what are the implications
of calling uvm_vnp_setsize/uncache() after VOP_FSYNC(), which might fail.
So I'd rather play safe and go with that diff.
> > Index: ufs/ffs/ffs_inode.c
> > ===
> > RCS file: /cvs/src/sys/ufs/ffs/ffs_inode.c,v
> > retrieving revision 1.81
> > diff -u -p -r1.81 ffs_inode.c
> > --- ufs/ffs/ffs_inode.c 12 Dec 2021 09:14:59 - 1.81
> > +++ ufs/ffs/ffs_inode.c 4 May 2022 15:32:15 -
> > @@ -172,11 +172,12 @@ ffs_truncate(struct inode *oip, off_t le
> > if (length > fs->fs_maxfilesize)
> > return (EFBIG);
> >
> > - uvm_vnp_setsize(ovp, length);
> > oip->i_ci.ci_lasta = oip->i_ci.ci_clen
> > = oip->i_ci.ci_cstart = oip->i_ci.ci_lastw = 0;
> >
> > if (DOINGSOFTDEP(ovp)) {
> > + uvm_vnp_setsize(ovp, length);
> > + (void) uvm_vnp_uncache(ovp);
> > if (length > 0 || softdep_slowdown(ovp)) {
> > /*
> > * If a file is only partially truncated, then
> >
> >
Re: ffs_truncate: Missing uvm_vnp_uncache() w/ softdep
> Date: Tue, 24 May 2022 14:28:39 +0200
> From: Martin Pieuchot
>
> The softdep code path is missing a UVM cache invalidation compared to
> the !softdep one. This is necessary to flush pages of a persisting
> vnode.
>
> Since uvm_vnp_setsize() is also called later in this function for the
> !softdep case move it to not call it twice.
>
> ok?
I'm not sure this is correct. I'm trying to understand why you're
moving the uvm_uvn_setsize() call. Are you just trying to call it
twice? Or are you trying to avoid calling it at all when we end up in
an error path?
The way you moved it means we'll still call it twice for "partially
truncated" files with softdeps. At least the way I understand the
code is that the code will fsync the vnode and dropping down in the
"normal" non-softdep code that will call uvm_vnp_setsize() (and
uvn_vnp_uncache()) again. So maybe you should move the
uvm_uvn_setsize() call into the else case?
> Index: ufs/ffs/ffs_inode.c
> ===
> RCS file: /cvs/src/sys/ufs/ffs/ffs_inode.c,v
> retrieving revision 1.81
> diff -u -p -r1.81 ffs_inode.c
> --- ufs/ffs/ffs_inode.c 12 Dec 2021 09:14:59 - 1.81
> +++ ufs/ffs/ffs_inode.c 4 May 2022 15:32:15 -
> @@ -172,11 +172,12 @@ ffs_truncate(struct inode *oip, off_t le
> if (length > fs->fs_maxfilesize)
> return (EFBIG);
>
> - uvm_vnp_setsize(ovp, length);
> oip->i_ci.ci_lasta = oip->i_ci.ci_clen
> = oip->i_ci.ci_cstart = oip->i_ci.ci_lastw = 0;
>
> if (DOINGSOFTDEP(ovp)) {
> + uvm_vnp_setsize(ovp, length);
> + (void) uvm_vnp_uncache(ovp);
> if (length > 0 || softdep_slowdown(ovp)) {
> /*
>* If a file is only partially truncated, then
>
>
ffs_truncate: Missing uvm_vnp_uncache() w/ softdep
The softdep code path is missing a UVM cache invalidation compared to
the !softdep one. This is necessary to flush pages of a persisting
vnode.
Since uvm_vnp_setsize() is also called later in this function for the
!softdep case move it to not call it twice.
ok?
Index: ufs/ffs/ffs_inode.c
===
RCS file: /cvs/src/sys/ufs/ffs/ffs_inode.c,v
retrieving revision 1.81
diff -u -p -r1.81 ffs_inode.c
--- ufs/ffs/ffs_inode.c 12 Dec 2021 09:14:59 - 1.81
+++ ufs/ffs/ffs_inode.c 4 May 2022 15:32:15 -
@@ -172,11 +172,12 @@ ffs_truncate(struct inode *oip, off_t le
if (length > fs->fs_maxfilesize)
return (EFBIG);
- uvm_vnp_setsize(ovp, length);
oip->i_ci.ci_lasta = oip->i_ci.ci_clen
= oip->i_ci.ci_cstart = oip->i_ci.ci_lastw = 0;
if (DOINGSOFTDEP(ovp)) {
+ uvm_vnp_setsize(ovp, length);
+ (void) uvm_vnp_uncache(ovp);
if (length > 0 || softdep_slowdown(ovp)) {
/*
* If a file is only partially truncated, then
