On Tue, 23 Sep 2014 01:51:56 -0400 Tejun Heo <t...@kernel.org> wrote:
> On Tue, Sep 23, 2014 at 03:40:58PM +1000, NeilBrown wrote: > > > Oh, I meant the buffer seqfile read op writes to, so it depends on the > > > fact that the allocation is only on the first read? That seems > > > extremely brittle to me, especially for an issue which tends to be > > > difficult to reproduce. > > > > It is easy for user-space to ensure they read once before any critical > > time.. > > Sure, but it's a hard and subtle dependency on an extremely obscure > implementation detail. > > > > I'd much rather keep things direct and make it explicitly allocate r/w > > > buffer(s) on open and disallow seq_file operations on such files. > > > > As far as I can tell, seq_read is used on all sysfs files that are > > readable except for 'binary' files. Are you suggesting all files that might > > need to be accessed without a kmalloc have to be binary files? > > kernfs ->direct_read() callback doesn't go through seq_file. sysfs > can be extended to support that for regular files, I guess. Or just > make those special files binary? > > > Having to identify those files which are important in advance seems the more > > "brittle" approach to me. I would much rather it "just worked" > > I disagree. The files which shouldn't involve memory allocations must > be identified no matter what. They're *very* special. And the rules > that userland has to follow seem completely broken to me. "Small" > writes are okay, whatever that means, and "small" reads are okay too > as long as it isn't the first read. Ooh, BTW, if the second read ends > up expanding the initial buffer, it isn't okay - the initial boundary > is PAGE_SIZE and the buffer is expanded twice on each overflow. How > are these rules okay? This is borderline crazy. In addition, the > read path involves a lot more code this way. It ends up locking down > buffer policies of the whole seqfile implementation. > > > Would you prefer a new per-attribute flag which directed sysfs to > > pre-allocate a full page, or a 'max_size' attribute which caused a buffer of > > that size to be allocated on open? > > The same size would be used to pre-allocate the seqfile buf (like > > single_open_size does) if reads were supported. > > Yes but I really think we should avoid seqfile dependency. I'll see what I can do. You didn't say if you preferred a flag or a 'max_size'. Thanks, NeilBrown
signature.asc
Description: PGP signature
