On Thu, Apr 25, 2019 at 3:14 PM Mimi Zohar <zohar@xxxxxxxxxxxxx> wrote:
> > The pieces that we did:
> > - initialize hashes correctly on open()
> > - hook 'sync'
> > - hook 'msync'
> > - hook 'truncate'
> > - introduce a per-cpu workqueue that gets work items from write and
> > dirty page flush. Long-running write is allowed to go as is (no
> > performance penalty from re-measurements), but once the the write goes
> > silent workqueue item is flushed and the file is hashed.
>
> What type of workqueue (eg. fifo)? Are all of the writes needed, as
> they might be re-written later.
We can talk about this, but what I have now is
static struct workqueue_struct *ima_update_wq;
ima_update_wq = create_workqueue("ima-update-wq");
..
queue_work(ima_update_wq, &iint->ima_update_data.work);
All writes are not hashed at all, it tries to insert an element into
the workqueue on every write, but if there is already one it is happy.
Alternative is that we add delayed work that is constantly kicked
forward, so rehash will fire for example 'dirty_expire_centisecs'
after the last write on the inode. Unfortunately I need really high
crash tolerance in my use case (lovely vendor kernel), so I don't
delay it much in our case.
Android is pretty rough test, btw. It keeps 800+ files open for
writing on /data alone. This survives that kind of thing crashing in a
loop.
> > Before I uplift this construction against the mainline, any thoughts
> > about this? All I can say so far is that it runs and it seems to keep
> > the system relatively crash tolerant. 'Don't let the perfect be the
> > enemy of the better' I guess...
>
> As long as the iint cache info isn't affected by these changes, the
> change is limited to writing out the xattr more frequently. If the
> iint cache info is affected, then you need to be careful that multiple
> readers/writers continue to work.
Sadly, it is affected. That is where I hide the 'work_struct' and the
'file' pointer :-/
--
Janne
