Re: [PATCH 0/2] shmem: Notify user space when file system is full

[Gabriel Krisman Bertazi <krisman@collabora.com>]

Amir Goldstein <amir73il@gmail.com> writes:

> Two things bother me about this proposal.
> One is that it makes more sense IMO to report ENOSPC events
> from vfs code.

Hi Amir,

I reimplemented this with FS_WB_ERROR in the branch below. It reports
writeback errors on mapping_set_error, as suggested.

  https://gitlab.collabora.com/krisman/linux/-/tree/wb-error

It is a WIP, and I'm not proposing it yet, cause I'm thinking about the
ENOSPC case a bit more...

> Why should the requirement to monitor ENOSPC conditions be specific to tmpfs?
> Especially, as I mentioned, there are already wrappers in place to report
> writeback errors on an inode (mapping_set_error), where the fsnotify hook
> can fit nicely.

mapping_set_error would trigger the ENOSPC event only when it happens on
an actual writeback error (i.e. BLK_STS_NOSPC), which is not the main
case I'm solving here.  In fact, most of the time, -ENOSPC will happen
before any IO is submitted, for instance, if an inode could not be
allocated during .create() or a block can't be allocated in
.write_begin(). In this case, it isn't really a writeback error
(semantically), and it is not registered as such by any file system.

We can treat those under the same FAN_WB_ERROR mask, but that is a bit
weird.  Maybe we should have a mask specifically for ENOSPC, or a,
"IO error" mask?

The patchset is quite trivial, but it has to touch many places in the VFS
(say, catch ENOSPC on create, fallocate, write, writev, etc). Please,
see the branch above to what that would look like.

Is that a viable solution?  Are you ok with reporting those cases under
the same writeback mask?

Btw, I'm thinking it could be tidy to transform many of those VFS calls,
from

	if (!error)
		fsnotify_modify(file);
	else if (error == -ENOSPC)
		fsnotify_nospace(file);

Into unconditionally calling the fsnotify_modify hook, which sends
the correct event depending on the operation result:

	void fsnotify_modify(int error, struct file *file)
	{
		if (likely(!error)) {
			fsnotify_file(file, FS_MODIFY);
	        } else if (error == -ENOSPC) {
	        	fsnotify_wb_error(file);
	        }
	}

(same for fsnotify_mkdir, fsnotify_open, etc).

If you are ok with that?


> I understand that you wanted to differentiate errors caused by memory
> pressure, but I don't understand why it makes sense for a filesystem monitor
> to get a different feedback than the writing application.

Maybe the differentiation of those two cases could be done as part of
the file system specific payload that I wanted to write for
FAN_FS_ERROR?  If so, it would be easier for the ENOSPC event trigger to
be implemented at the filesystem-level.

> The second thing that bothers me is that I think the ENOSPC condition
> should not be reported on the same event mask as filesystem corruption
> condition because it seems like a valid use case for filesystem monitor
> to want to be notified about corruption and not about ENOSPC.

-- 
Gabriel Krisman Bertazi