lkml.org 
[lkml]   [2010]   [Feb]   [2]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
SubjectRe: [PATCH][RFC] %pd - for printing dentry name
From
Date
Al Viro <viro@ZenIV.linux.org.uk> writes:

> On Mon, Feb 01, 2010 at 09:55:42PM -0800, Eric W. Biederman wrote:
>
>> We already have rename_lock, which is only take for write in d_move_locked.
>>
>> I wonder if there is an instruction sequence that could guarantee that the
>> string copy is done atomically from the perspective of another cpu,
>> d_iname fits nicely on a single cache line so it should be possible.
>>
>> That is a stronger guarantee than we need. All we really need is the
>> guarantee that a reader will see the string null terminator. dentries already
>> have rcu safe lifetimes.
>>
>> Hmm.
>>
>> We should be able to do:
>> struct qstr *name;
>> int len;
>> char buf[MAX_LEN + 1];
>> long seq;
>>
>> do {
>> seq = read_seqbegin(&rename_lock);
>> rcu_read_lock();
>> name = rcu_dereference(dentry->d_name.name);
>> len = dentry->d_name.len;
>> if (read_seqretry(&rename_lock, seq)
>> continue;
>> if (len > MAX_LEN)
>> len = MAX_LEN;
>> memcpy(buf, name, len);
>> buf[len] = '\0';
>> rcu_read_unlock();
>> } while (read_seqretry(&rename_lock, seq));
>
> Actually, WTF do we bother? seqlock writer grabs embedded spinlock.
> So can we, since *that* is far more narrow than ->d_lock and it's
> static, so it's not like somebody outside of dcache.c can decide
> to grab.
>
> We could just inline string() into dname_string() and take the entire
> thing to fs/dcache.c. And protect it either with direct locking
> of rename_lock.lock or another seq_writelock; contention is not
> an issue, since if printk guts are your hotpath you are already
> FUBAR.
>
> Something like that (completely untested):

If we are going to take a lock this seems as sane as any.

Do we want to honor oops_in_progress aka bust_spinlocks here?

Perhaps just:
if (oops_in_progress)
return buf;

To guarantee we get the rest of a panic message out of the kernel.

Eric


>
> diff --git a/fs/dcache.c b/fs/dcache.c
> index 953173a..a4d30bc 100644
> --- a/fs/dcache.c
> +++ b/fs/dcache.c
> @@ -1703,6 +1703,48 @@ void d_move(struct dentry * dentry, struct dentry * target)
> spin_unlock(&dcache_lock);
> }
>
> +/*
> + * for vsprintf use only.
> + */
> +char *dname_string(char *buf, char *end, struct dentry *d,
> + int precision, int width, int left_align)
> +{
> + int len, i;
> + const unsigned char *s;
> +
> + write_seqlock(&rename_lock);
> + if (precision > d->d_name.len)
> + precision = d->d_name.len;
> +
> + s = d->d_name.name;
> + if ((unsigned long)s < PAGE_SIZE)
> + s = "(null)";
> +
> + len = strnlen(s, precision);
> +
> + if (!left_align) {
> + while (len < width--) {
> + if (buf < end)
> + *buf = ' ';
> + ++buf;
> + }
> + }
> + for (i = 0; i < len; ++i) {
> + if (buf < end)
> + *buf = *s;
> + ++buf; ++s;
> + }
> +
> + while (len < width--) {
> + if (buf < end)
> + *buf = ' ';
> + ++buf;
> + }
> +
> + write_sequnlock(&rename_lock);
> + return buf;
> +}
> +
> /**
> * d_ancestor - search for an ancestor
> * @p1: ancestor dentry
> diff --git a/include/linux/dcache.h b/include/linux/dcache.h
> index 30b93b2..2dc286a 100644
> --- a/include/linux/dcache.h
> +++ b/include/linux/dcache.h
> @@ -379,5 +379,6 @@ extern struct vfsmount *lookup_mnt(struct path *);
> extern struct dentry *lookup_create(struct nameidata *nd, int is_dir);
>
> extern int sysctl_vfs_cache_pressure;
> +extern char *dname_string(char *, char *, struct dentry *, int, int, int);
>
> #endif /* __LINUX_DCACHE_H */
> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> index 3b8aeec..ab0e40a 100644
> --- a/lib/vsprintf.c
> +++ b/lib/vsprintf.c
> @@ -915,6 +915,8 @@ static char *uuid_string(char *buf, char *end, const u8 *addr,
> * [0][1][2][3]-[4][5]-[6][7]-[8][9]-[10][11][12][13][14][15]
> * little endian output byte order is:
> * [3][2][1][0]-[5][4]-[7][6]-[8][9]-[10][11][12][13][14][15]
> + * - 'd' For dentry name. NOTE: don't use under dentry->d_lock, there
> + * you can safely use ->d_name.name instead.
> *
> * Note: The difference between 'S' and 'F' is that on ia64 and ppc64
> * function pointers are really function descriptors, which contain a
> @@ -958,6 +960,9 @@ static char *pointer(const char *fmt, char *buf, char *end, void *ptr,
> break;
> case 'U':
> return uuid_string(buf, end, ptr, spec, fmt);
> + case 'd':
> + return dname_string(buf, end, ptr, spec.precision,
> + spec.field_width, spec.flags & LEFT);
> }
> spec.flags |= SMALL;
> if (spec.field_width == -1) {
> @@ -1191,6 +1196,7 @@ qualifier:
> * http://tools.ietf.org/html/draft-ietf-6man-text-addr-representation-00
> * %pU[bBlL] print a UUID/GUID in big or little endian using lower or upper
> * case.
> + * %pd print dentry name
> * %n is ignored
> *
> * The return value is the number of characters which would


\
 
 \ /
  Last update: 2010-02-02 20:23    [W:0.130 / U:0.216 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site